[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH v5] lib/ukblkdev: Stop and release a Unikraft block device



Hi Roxana,

On 16.10.19 11:27, Roxana Nicolescu wrote:
Hi Simon,

I cannot read the `state` after calling the driver `unconfigure` function,
because in there all the structures are freed.
The state won't be a valid variable since its memory is freed.
In my opinion, this variable is just here for visibility, it doesn't help so much for something else,
since it won't be used after calling `blkdev_unconfigure`.

The state variable should be still available. unconfigure should only free the resources that were allocated by the driver when transiting from 'UNCONFIGURED' to 'CONFIGURED' state. Since the state variable is part of the library-managed _data structure it should be still available. On `unregister`, _data should be free'd, which you are actually doing as far as I can see.

Thanks,

Simon


Roxana

On Tue, Oct 15, 2019 at 4:52 PM Simon Kuenzer <simon.kuenzer@xxxxxxxxx <mailto:simon.kuenzer@xxxxxxxxx>> wrote:

    Hey, thanks a lot for the quick fix. A minor comment.

    Thanks,

    Simon

    On 15.10.19 15:41, Roxana Nicolescu wrote:
     > A device can be stopped, which will not permit further requests.
     > Also, for closing the device, the device must pe stopped,
     > each queue must be release and all the data allocated must be freed.
     >
     > Signed-off-by: Roxana Nicolescu <nicolescu.roxana1996@xxxxxxxxx
    <mailto:nicolescu.roxana1996@xxxxxxxxx>>
     > ---
     >   lib/ukblkdev/blkdev.c                   | 90
    +++++++++++++++++++++++++
>   lib/ukblkdev/exportsyms.uk <http://exportsyms.uk> |  4 ++
     >   lib/ukblkdev/include/uk/blkdev.h        | 44 ++++++++++++
     >   lib/ukblkdev/include/uk/blkdev_core.h   | 13 ++++
     >   lib/ukblkdev/include/uk/blkdev_driver.h |  9 +++
     >   5 files changed, 160 insertions(+)
     >
     > diff --git a/lib/ukblkdev/blkdev.c b/lib/ukblkdev/blkdev.c
     > index 8679e074..11e33b59 100644
     > --- a/lib/ukblkdev/blkdev.c
     > +++ b/lib/ukblkdev/blkdev.c
     > @@ -466,3 +466,93 @@ int uk_blkdev_sync_io(struct uk_blkdev *dev,
     >       return req->result;
     >   }
     >   #endif
     > +
     > +int uk_blkdev_stop(struct uk_blkdev *dev)
     > +{
     > +     int rc = 0;
     > +
     > +     UK_ASSERT(dev);
     > +     UK_ASSERT(dev->_data);
     > +     UK_ASSERT(dev->dev_ops);
     > +     UK_ASSERT(dev->dev_ops->dev_stop);
     > +     UK_ASSERT(dev->_data->state == UK_BLKDEV_RUNNING);
     > +
     > +     uk_pr_info("Trying to stop blkdev%"PRIu16" device\n",
     > +                     dev->_data->id);
     > +     rc = dev->dev_ops->dev_stop(dev);
     > +     if (rc)
     > +             uk_pr_err("Failed to stop blkdev%"PRIu16" device %d\n",
     > +                             dev->_data->id, rc);
     > +     else {
     > +             uk_pr_info("Stopped blkdev%"PRIu16" device\n",
     > +                             dev->_data->id);
     > +             dev->_data->state = UK_BLKDEV_CONFIGURED;
     > +     }
     > +
     > +     return rc;
     > +}
     > +
     > +int uk_blkdev_queue_release(struct uk_blkdev *dev, uint16_t
    queue_id)
     > +{
     > +     int rc = 0;
     > +
     > +     UK_ASSERT(dev != NULL);
     > +     UK_ASSERT(dev->_data);
     > +     UK_ASSERT(dev->dev_ops);
     > +     UK_ASSERT(dev->dev_ops->queue_release);
     > +     UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
     > +     UK_ASSERT(dev->_data->state != UK_BLKDEV_RUNNING);
     > +     UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
     > +
     > +#if CONFIG_LIBUKBLKDEV_DISPATCHERTHREADS
     > +     if (dev->_data->queue_handler[queue_id].callback)
> +  _destroy_event_handler(&dev->_data->queue_handler[queue_id]);
     > +#endif
     > +
     > +     rc = dev->dev_ops->queue_release(dev, dev->_queue[queue_id]);
     > +     if (rc)
     > +             uk_pr_err("Failed to release
    blkdev%"PRIu16"-q%"PRIu16": %d\n",
     > +                             dev->_data->id, queue_id, rc);
     > +     else {
     > +             uk_pr_info("Released blkdev%"PRIu16"-q%"PRIu16"\n",
     > +                             dev->_data->id, queue_id);
     > +
     > +             dev->_queue[queue_id] = NULL;
     > +     }
     > +
     > +     return rc;
     > +}
     > +
     > +void uk_blkdev_drv_unregister(struct uk_blkdev *dev)
     > +{
     > +     uint16_t id;
     > +
     > +     UK_ASSERT(dev != NULL);
     > +     UK_ASSERT(dev->_data);
     > +     UK_ASSERT(dev->_data->state == UK_BLKDEV_UNCONFIGURED);
     > +
     > +     id = dev->_data->id;
     > +
     > +     uk_free(dev->_data->a, dev->_data);
     > +     UK_TAILQ_REMOVE(&uk_blkdev_list, dev, _list);
     > +     blkdev_count--;
     > +
     > +     uk_pr_info("Unregistered blkdev%"PRIu16": %p\n",
     > +                     id, dev);
     > +}
     > +
     > +int uk_blkdev_unconfigure(struct uk_blkdev *dev)
     > +{
     > +     uint16_t q_id;
     > +
     > +     UK_ASSERT(dev);
     > +     UK_ASSERT(dev->_data);
     > +     UK_ASSERT(dev->dev_ops);
     > +     UK_ASSERT(dev->dev_ops->dev_unconfigure);
     > +     UK_ASSERT(dev->_data->state == UK_BLKDEV_CONFIGURED);
     > +     for (q_id = 0; q_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES; ++q_id)
     > +             UK_ASSERT(PTRISERR(dev->_queue[q_id]));
     > +
     > +     dev->_data->state = UK_BLKDEV_UNCONFIGURED;

    You should only go into UNCONFIGURED state, if you did not get an
    error,
    from the driver right?

     > +     return dev->dev_ops->dev_unconfigure(dev);
     > +}
     > diff --git a/lib/ukblkdev/exportsyms.uk <http://exportsyms.uk>
    b/lib/ukblkdev/exportsyms.uk <http://exportsyms.uk>
     > index 5c05994a..f3be6c51 100644
     > --- a/lib/ukblkdev/exportsyms.uk <http://exportsyms.uk>
     > +++ b/lib/ukblkdev/exportsyms.uk <http://exportsyms.uk>
     > @@ -12,3 +12,7 @@ uk_blkdev_start
     >   uk_blkdev_queue_submit_one
     >   uk_blkdev_queue_finish_reqs
     >   uk_blkdev_sync_io
     > +uk_blkdev_stop
     > +uk_blkdev_queue_release
     > +uk_blkdev_drv_unregister
     > +uk_blkdev_unconfigure
     > diff --git a/lib/ukblkdev/include/uk/blkdev.h
    b/lib/ukblkdev/include/uk/blkdev.h
     > index ad0f2629..e1e9b069 100644
     > --- a/lib/ukblkdev/include/uk/blkdev.h
     > +++ b/lib/ukblkdev/include/uk/blkdev.h
     > @@ -471,6 +471,50 @@ int uk_blkdev_sync_io(struct uk_blkdev *dev,
     >
     >   #endif
     >
     > +/**
     > + * Stop a Unikraft block device, and set its state to
    UK_BLKDEV_CONFIGURED
     > + * state. From now on, users cannot send any requests.
     > + * If there are pending requests, this function will return
    -EBUSY because
     > + * the queues are not empty. If polling is used instead of
    interrupts,
     > + * make sure to clean the queue and process all the responses
    before this
     > + * operation is called.
     > + * The device can be restarted with a call to uk_blkdev_start().
     > + *
     > + * @param dev
     > + *   The Unikraft Block Device.
     > + * @return
     > + *   - 0: Success
     > + *   - (<0): on error returned by driver
     > + */
     > +int uk_blkdev_stop(struct uk_blkdev *dev);
     > +
     > +/**
     > + * Free a queue and its descriptors for an Unikraft block device.
     > + * @param dev
     > + *   The Unikraft Block Device.
     > + * @param queue_id
     > + *   The index of the queue to set up.
     > + *   The value must be in range [0, nb_queue -1] previously supplied
     > + *   to uk_blkdev_configure()
     > + *   @return
     > + *   - 0: Success
     > + *   - (<0): on error returned by driver
     > + */
     > +int uk_blkdev_queue_release(struct uk_blkdev *dev, uint16_t
    queue_id);
     > +
     > +/**
     > + * Close a stopped Unikraft block device.
     > + * The function frees all resources except for
     > + * the ones needed by the UK_BLKDEV_UNCONFIGURED state.
     > + *
     > + * @param dev
     > + *   The Unikraft Block Device.
     > + * @return
     > + *   - 0: Success
     > + *   - (<0): on error returned by driver
     > + */
     > +int uk_blkdev_unconfigure(struct uk_blkdev *dev);
     > +
     >   #ifdef __cplusplus
     >   }
     >   #endif
     > diff --git a/lib/ukblkdev/include/uk/blkdev_core.h
    b/lib/ukblkdev/include/uk/blkdev_core.h
     > index b12e3eb1..ba9bf575 100644
     > --- a/lib/ukblkdev/include/uk/blkdev_core.h
     > +++ b/lib/ukblkdev/include/uk/blkdev_core.h
     > @@ -203,14 +203,27 @@ typedef int
    (*uk_blkdev_queue_submit_one_t)(struct uk_blkdev *dev,
     >   typedef int (*uk_blkdev_queue_finish_reqs_t)(struct uk_blkdev *dev,
     >               struct uk_blkdev_queue *queue);
     >
     > +/** Driver callback type to stop an Unikraft block device. */
     > +typedef int (*uk_blkdev_stop_t)(struct uk_blkdev *dev);
     > +
     > +/** Driver callback type to release a queue of an Unikraft block
    device. */
     > +typedef int (*uk_blkdev_queue_release_t)(struct uk_blkdev *dev,
     > +             struct uk_blkdev_queue *queue);
     > +
     > +/** Driver callback type to close an Unikraft block device. */
     > +typedef int (*uk_blkdev_unconfigure_t)(struct uk_blkdev *dev);
     > +
     >   struct uk_blkdev_ops {
     >       uk_blkdev_get_info_t                            get_info;
     >       uk_blkdev_configure_t                           dev_configure;
     >       uk_blkdev_queue_get_info_t                      queue_get_info;
     >       uk_blkdev_queue_configure_t                     queue_setup;
     >       uk_blkdev_start_t                               dev_start;
     > +     uk_blkdev_stop_t                                dev_stop;
>       uk_blkdev_queue_intr_enable_t  queue_intr_enable; >       uk_blkdev_queue_intr_disable_t queue_intr_disable;
     > +     uk_blkdev_queue_release_t                       queue_release;
> +     uk_blkdev_unconfigure_t  dev_unconfigure;
     >   };
     >
     >   /**
     > diff --git a/lib/ukblkdev/include/uk/blkdev_driver.h
    b/lib/ukblkdev/include/uk/blkdev_driver.h
     > index a3c41da3..be807069 100644
     > --- a/lib/ukblkdev/include/uk/blkdev_driver.h
     > +++ b/lib/ukblkdev/include/uk/blkdev_driver.h
     > @@ -106,6 +106,15 @@ static inline void
    uk_blkdev_drv_queue_event(struct uk_blkdev *dev,
     >   #define uk_blkreq_finished(req) \
     >       (ukarch_store_n(&(req)->state.counter, UK_BLKDEV_REQ_FINISHED))
     >
     > +/**
     > + * Frees the data allocated for the Unikraft Block Device.
     > + * Removes the block device from the list.
     > + *
     > + * @param dev
     > + *   Unikraft block device
     > + */
     > +void uk_blkdev_drv_unregister(struct uk_blkdev *dev);
     > +
     >   #ifdef __cplusplus
     >   }
     >   #endif
     >


_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.