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

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



On 22.10.19 16:55, Roxana Nicolescu wrote:
Hi Simon,

See inline.

Roxana

On 22.10.2019 17:04, Simon Kuenzer wrote:
On 22.10.19 11:32, Roxana Nicolescu wrote:
Hi Simon,


Sorry for the delay.

See inline.


Roxana

On 18.10.2019 15:26, Simon Kuenzer wrote:
Hey Roxana,

thanks a lot for the update. I double-checked uk_blkdev_queue_release() and found two things. If you want, I can adopt them while upstreaming. But please let me know that this is inline with with you.

Thanks,

Simon

On 17.10.19 17:11, Roxana Nicolescu wrote:
We introduce 3 main function for cleaning up the device:
    -> stop: further requests are not allowed
    -> queue release: release every data used for each queue
    -> unconfigure: sets the device state to UNCONFIGURED.

Unregister the device is also introduced, which is responsible for
cleaning up any memory used by uk_blkdev.

Signed-off-by: Roxana Nicolescu <nicolescu.roxana1996@xxxxxxxxx>
---
  lib/ukblkdev/blkdev.c                   | 99 +++++++++++++++++++++++++
  lib/ukblkdev/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, 169 insertions(+)

diff --git a/lib/ukblkdev/blkdev.c b/lib/ukblkdev/blkdev.c
index 8679e074..06b47438 100644
--- a/lib/ukblkdev/blkdev.c
+++ b/lib/ukblkdev/blkdev.c
@@ -466,3 +466,102 @@ 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);

The assertion should be (dev->_data->state == UK_BLKDEV_CONFIGURED), right? If you confirm it, I can change it during upstreaming.

You are right.
+ UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
+

Destroying the handler would make sense after we know the release was successful - right? Probably we should put a TODO comment that says that we actually should pause the thread first, release the queue and then destroy the thread. If queue release fails we could just unpause the dispatcher thread again. At least I would move the dispatcher destroying to the success case.

Can you give me an edge case for this? In case the queue is not released, do we have a specific situation where you can recover from an error? If so, you are right. The thread shouldn't be destroyed, in case `queue_release` is called again.

It is because of two reasons actually: Consistency and queue_release being a non-void operation. So far, we tried in the ukblkdev API to stay in the current state in case of errors. Everything is kept as if the function was never called. Because queue_release is non-void, it forces us to handle the error case properly. In fact, I expect that most drivers will never fail but we still did not put the driver callback as `void`.

Actually, I think it's easier to destroy the thread after calling `queue_release` from the driver. Because the thread is not doing any processing, since in the `stop` function we make sure every queue is empty (every response is processed). What do you think?

Yes. You stop the thread when the driver returns success on queue_release(). You are right, this should be enough since it is our thread.

+#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;
+    int rc;
+
+    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]));
+
+    rc = dev->dev_ops->dev_unconfigure(dev);
+    if (rc)
+        uk_pr_err("Failed to unconfigure blkdev%"PRIu16": %d\n",
+                dev->_data->id, rc);
+    else {
+        uk_pr_info("Unconfigured blkdev%"PRIu16"\n", dev->_data->id);
+        dev->_data->state = UK_BLKDEV_UNCONFIGURED;
+    }
+
+    return rc;
+}
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..ad60cbec 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.
+ * The device can be reconfigured with a call to uk_blkdev_configure().
+ * @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®.