[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH 6/6] lib/ukblkdev: Stop and release an Unikraft block device
Hi Simon,
See my comments below.
Roxana On 29.05.19 10:34, 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>
> ---
> lib/ukblkdev/blkdev.c | 85 +++++++++++++++++++++++++++++++++
> lib/ukblkdev/exportsyms.uk | 4 ++
> lib/ukblkdev/include/uk/blkdev.h | 31 ++++++++++++
> lib/ukblkdev/include/uk/blkdev_core.h | 13 +++++
> lib/ukblkdev/include/uk/blkdev_driver.h | 11 +++++
> 5 files changed, 144 insertions(+)
>
> diff --git a/lib/ukblkdev/blkdev.c b/lib/ukblkdev/blkdev.c
> index 534a9ae1..d06b5250 100644
> --- a/lib/ukblkdev/blkdev.c
> +++ b/lib/ukblkdev/blkdev.c
> @@ -467,3 +467,88 @@ int uk_blkdev_sync_io(struct uk_blkdev *dev,
> return req->result_status;
> }
> #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);
> +
> + 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;
> +}
> +
> +void 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);
> +
> +#if CONFIG_LIBUKBLKDEV_DISPATCHERTHREADS
> + if (dev->_data->queue_handler[queue_id].callback)
> + _destroy_event_handler(&dev->_data->queue_handler[queue_id]);
Hum not sure if there is a condition where this could crash. Maybe you
should get the queue thread into an defined state and destroy it there.
I am just thinking what happens when it is getting destroyed while
executing the registered event callback. Maybe we need a condition
variable next to the event semaphore which is checked if the thread
should exit by itself.
At least we should put a /* TODO */ comment here because we may run into
problems especially with preemptive scheduling. It is okay to solve this
problem later.
I will provide a variable to stop the while loop in the dispatcher thread in v2.
> +#endif
> +
> + rc = dev->dev_ops->queue_release(dev, queue_id);
Use `struct uk_blkdev_queue *` also here as commented in patch 4/6.
> + 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;
> + }
> +}
> +
> +void uk_blkdev_drv_unregister(struct uk_blkdev *dev, struct uk_alloc *a)
> +{
> + uint16_t id;
> +
> + UK_ASSERT(dev != NULL);
> + UK_ASSERT(dev->_data);
> + UK_ASSERT(dev->_data->state != UK_BLKDEV_RUNNING);
> +
> + id = dev->_data->id;
> +
> + uk_free(a, dev->_data);
Maybe it is better to store `uk_alloc *a` on `_data` while allocating.
Then we would not need the allocator function argument anymore.
For me, it is the same thing. But I will change it in v2.
> + UK_TAILQ_REMOVE(&uk_blkdev_list, dev, _list);
> + blkdev_count--;
> +
> + uk_pr_info("Unregistered blkdev%"PRIu16": %p\n",
> + id, dev);
> +}
> +
> +void uk_blkdev_close(struct uk_blkdev *dev)
> +{
> + uint16_t id;
> +
> + UK_ASSERT(dev);
> + UK_ASSERT(dev->_data);
> + UK_ASSERT(dev->dev_ops);
> + UK_ASSERT(dev->dev_ops->dev_close);
> + UK_ASSERT(dev->_data->state != UK_BLKDEV_RUNNING);
> +
> + id = dev->_data->id;
> + dev->dev_ops->dev_close(dev);
> +
> + uk_pr_info("Closed blkdev%"PRIu16"\n", id);
> +}
> diff --git a/lib/ukblkdev/exportsyms.uk b/lib/ukblkdev/exportsyms.uk
> index 0274dae8..0abd8e10 100644
> --- a/lib/ukblkdev/exportsyms.uk
> +++ b/lib/ukblkdev/exportsyms.uk
> @@ -22,3 +22,7 @@ uk_blkdev_queue_finish_reqs
> uk_blkdev_sync_io
> uk_blkdev_read
> uk_blkdev_write
> +uk_blkdev_stop
> +uk_blkdev_queue_release
> +uk_blkdev_drv_unregister
> +uk_blkdev_close
> diff --git a/lib/ukblkdev/include/uk/blkdev.h b/lib/ukblkdev/include/uk/blkdev.h
> index ba4c2784..00789198 100644
> --- a/lib/ukblkdev/include/uk/blkdev.h
> +++ b/lib/ukblkdev/include/uk/blkdev.h
> @@ -468,6 +468,37 @@ int uk_blkdev_sync_io(struct uk_blkdev *dev,
>
> #endif
>
> +/**
> + * Stop an Unikraft block device, and set its state to UK_BLKDEV_CONFIGURED
> + * state.
> + * The device can be restarted with a call to uk_blkdev_start().
> + *
> + * @param dev
> + * The Unikraft Block Device.
> + */
> +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()
> + */
> +void uk_blkdev_queue_release(struct uk_blkdev *dev, uint16_t queue_id);
Use `struct uk_blkdev_queue *` also here.
Actually, I chose to use the queue_id in here, for debugging / error prints.
> +
> +/**
> + * 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.
> + */
> +void uk_blkdev_close(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 66831f90..71fdd34b 100644
> --- a/lib/ukblkdev/include/uk/blkdev_core.h
> +++ b/lib/ukblkdev/include/uk/blkdev_core.h
> @@ -204,6 +204,16 @@ typedef int (*uk_blkdev_queue_submit_one_t)(struct uk_blkdev *dev,
> typedef int (*uk_blkdev_queue_finish_reqs_t)(struct uk_blkdev *dev,
> uint16_t queue_id);
>
> +/** 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,
> + uint16_t queue_id);
Use `struct uk_blkdev_queue *` also here as commented in patch 4/6.
> +
> +/** Driver callback type to close an Unikraft block device. */
> +typedef void (*uk_blkdev_close_t)(struct uk_blkdev *dev);
> +
> struct uk_blkdev_ops {
> uk_blkdev_get_info_t get_info;
> uk_blkdev_configure_t dev_configure;
> @@ -211,7 +221,10 @@ struct uk_blkdev_ops {
> uk_blkdev_queue_configure_t queue_setup;
> uk_blkdev_queue_intr_enable_t queue_intr_enable;
> uk_blkdev_start_t dev_start;
> + uk_blkdev_stop_t dev_stop;
> uk_blkdev_queue_intr_disable_t queue_intr_disable;
> + uk_blkdev_queue_release_t queue_release;
> + uk_blkdev_close_t dev_close;
> };
>
> /**
> diff --git a/lib/ukblkdev/include/uk/blkdev_driver.h b/lib/ukblkdev/include/uk/blkdev_driver.h
> index 37b5a3e8..f361e8fb 100644
> --- a/lib/ukblkdev/include/uk/blkdev_driver.h
> +++ b/lib/ukblkdev/include/uk/blkdev_driver.h
> @@ -98,6 +98,17 @@ static inline void uk_blkdev_drv_queue_event(struct uk_blkdev *dev,
> #endif
> }
>
> +/**
> + * Frees the data allocated for the Unikraft Block Device.
> + * Removes the block device from the list.
> + *
> + * @param dev
> + * Unikraft block device
> + * @param a
> + * Allocator used for libukblkdev private data (dev->_data).
> + */
> +void uk_blkdev_drv_unregister(struct uk_blkdev *dev, struct uk_alloc *a);
> +
> #ifdef __cplusplus
> }
> #endif
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
