[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH v3 6/6] lib/ukblkdev: Stop and release an Unikraft block device
Hey Roxana,
I already upstreamed patch 1-5 of this series since they form already a
complete and working blkdev API. On this patch I still have a few
questions. Please let me know you thoughts.
Thanks a lot for your work!
Simon
On 06.10.19 14:22, 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 | 101 ++++++++++++++++++++++++
lib/ukblkdev/exportsyms.uk | 4 +
lib/ukblkdev/include/uk/blkdev.h | 45 +++++++++++
lib/ukblkdev/include/uk/blkdev_core.h | 13 +++
lib/ukblkdev/include/uk/blkdev_driver.h | 9 +++
5 files changed, 172 insertions(+)
diff --git a/lib/ukblkdev/blkdev.c b/lib/ukblkdev/blkdev.c
index 8679e074..5cad2ec2 100644
--- a/lib/ukblkdev/blkdev.c
+++ b/lib/ukblkdev/blkdev.c
@@ -466,3 +466,104 @@ 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 id;
+ 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_RUNNING);
Shouldn't the assertion check for UK_BLKDEV_CONFIGURED state?
+
+ id = dev->_data->id;
+ dev->_data->state = UK_BLKDEV_UNCONFIGURED;
+ for (q_id = 0; q_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES; ++q_id) {
+ if (dev->_queue[q_id]) {
+ uk_pr_err("Failed to unconfigure blkdev%"PRIu16":
Queue%"PRIu16"not closed",
+ id, q_id);
+ return -EBUSY;
+ }
+ }
Yes, you are right, according to my previous comment, you would
implement it like this. My original intention was actually that I wanted
to double check with you if dev->dev_ops->dev_unconfigure(dev) should be
really void and not return anything. I chose a bad example, because
"when a queue is still configured" should be treated as API mis-use. The
API user should have closed all of them in the previous step. I think
errors code come from the device drivers - for instance if communication
with the underlying device is lost. So, I would use this code instead:
for (q_id = 0; q_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES; ++q_id)
UK_ASSERT(PTRISERR(dev->_queue[queue_id]);
+
+ dev->dev_ops->dev_unconfigure(dev);
We probably should let dev_ops->dev_unconfigure() return an int and
forward it instead of returning 0.
+ uk_pr_info("Unconfigured blkdev%"PRIu16"\n", id);
+
+ return 0;
+}
diff --git a/lib/ukblkdev/exportsyms.uk b/lib/ukblkdev/exportsyms.uk
index 5c05994a..f3be6c51 100644
--- a/lib/ukblkdev/exportsyms.uk
+++ b/lib/ukblkdev/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..33c988ce 100644
--- a/lib/ukblkdev/include/uk/blkdev.h
+++ b/lib/ukblkdev/include/uk/blkdev.h
@@ -471,6 +471,51 @@ int uk_blkdev_sync_io(struct uk_blkdev *dev,
#endif
+/**
+ * Stop an 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 ||
+ * (-EBUSY) if not all the queues are released
+ */
+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..7931c5db 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 void (*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
|
