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

Re: [Minios-devel] [UNIKRAFT PATCH v2 4/6] lib/ukblkdev: Request interface



Hey Roxana,

thanks for the changes! I will take this patch and apply 3 minor modifications while upstreaming. They are basically about adding some more details within your comments. I think by having them, the API is becoming even more clear.

Thanks,

Simon

Reviewed-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>

On 01.07.19 12:04, Roxana Nicolescu wrote:
This patch introduces the interface for sending a request and receiving
a response. This is designed to support asynchronous operations.
The interface permits sector-wide access only.
Receiving the response from backend can be done with interrupts or
with polling.

Signed-off-by: Roxana Nicolescu <nicolescu.roxana1996@xxxxxxxxx>
---
  lib/ukblkdev/blkdev.c                   |  34 ++++++
  lib/ukblkdev/exportsyms.uk              |  10 ++
  lib/ukblkdev/include/uk/blkdev.h        | 199 ++++++++++++++++++++++++++++++++
  lib/ukblkdev/include/uk/blkdev_core.h   |  62 ++++++++++
  lib/ukblkdev/include/uk/blkdev_driver.h |  28 +++++
  lib/ukblkdev/include/uk/blkreq.h        | 112 ++++++++++++++++++
  6 files changed, 445 insertions(+)
  create mode 100644 lib/ukblkdev/include/uk/blkreq.h

diff --git a/lib/ukblkdev/blkdev.c b/lib/ukblkdev/blkdev.c
index c7098274..a92b29f1 100644
--- a/lib/ukblkdev/blkdev.c
+++ b/lib/ukblkdev/blkdev.c
@@ -84,6 +84,12 @@ int uk_blkdev_drv_register(struct uk_blkdev *dev, struct 
uk_alloc *a,
        UK_ASSERT(dev->dev_ops->queue_setup);
        UK_ASSERT(dev->dev_ops->get_info);
        UK_ASSERT(dev->dev_ops->queue_get_info);
+       UK_ASSERT(dev->submit_one);
+       UK_ASSERT(dev->finish_reqs);
+       UK_ASSERT((dev->dev_ops->queue_intr_enable &&
+                       dev->dev_ops->queue_intr_disable)
+                       || (!dev->dev_ops->queue_intr_enable
+                       && !dev->dev_ops->queue_intr_disable));
dev->_data = _alloc_data(a, blkdev_count, drv_name);
        if (!dev->_data)
@@ -374,3 +380,31 @@ int uk_blkdev_start(struct uk_blkdev *dev)
return rc;
  }
+
+int uk_blkdev_queue_submit_one(struct uk_blkdev *dev,
+               uint16_t queue_id,
+               struct uk_blkreq *req)
+{
+       UK_ASSERT(dev);
+       UK_ASSERT(dev->_data);
+       UK_ASSERT(dev->submit_one);
+       UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+       UK_ASSERT(dev->_data->state == UK_BLKDEV_RUNNING);
+       UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
+       UK_ASSERT(req != NULL);
+
+       return dev->submit_one(dev, dev->_queue[queue_id], req);
+}
+
+int uk_blkdev_queue_finish_reqs(struct uk_blkdev *dev,
+               uint16_t queue_id)
+{
+       UK_ASSERT(dev);
+       UK_ASSERT(dev->finish_reqs);
+       UK_ASSERT(dev->_data);
+       UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+       UK_ASSERT(dev->_data->state == UK_BLKDEV_RUNNING);
+       UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
+
+       return dev->finish_reqs(dev, dev->_queue[queue_id]);
+}
diff --git a/lib/ukblkdev/exportsyms.uk b/lib/ukblkdev/exportsyms.uk
index 077994f0..33402f78 100644
--- a/lib/ukblkdev/exportsyms.uk
+++ b/lib/ukblkdev/exportsyms.uk
@@ -9,3 +9,13 @@ uk_blkdev_configure
  uk_blkdev_queue_get_info
  uk_blkdev_queue_configure
  uk_blkdev_start
+uk_blkdev_queue_intr_enable
+uk_blkdev_queue_intr_disable
+uk_blkdev_capabilities
+uk_blkdev_ssize
+uk_blkdev_max_sec_per_req
+uk_blkdev_mode
+uk_blkdev_sectors
+uk_blkdev_ioalign

I did not notice last time that you do not need to add inline functions and macros to the `exportsyms.uk` file. Those definitions aren't creating any symbol in the library object. But on the other hand it does not hurt to have them listed here. It is just a statement without effect. If you want to remove it, you can do this with a follow-up patch. I will accept this one in order to move on with reviewing your block drivers.

+uk_blkdev_queue_submit_one
+uk_blkdev_queue_finish_reqs
diff --git a/lib/ukblkdev/include/uk/blkdev.h b/lib/ukblkdev/include/uk/blkdev.h
index 375407ed..b941911c 100644
--- a/lib/ukblkdev/include/uk/blkdev.h
+++ b/lib/ukblkdev/include/uk/blkdev.h
@@ -222,6 +222,205 @@ int uk_blkdev_queue_configure(struct uk_blkdev *dev, 
uint16_t queue_id,
   */
  int uk_blkdev_start(struct uk_blkdev *dev);
+/**
+ * Get the capabilities info which stores info about the device,
+ * like nb_of_sectors, sector_size etc
+ * The device state has to be UK_BLKDEV_RUNNING.
+ *
+ * @param dev
+ *     The Unikraft Block Device.
+ *
+ * @return
+ *     A pointer to a structure of type *uk_blkdev_capabilities*.
+ **/
+static inline const struct uk_blkdev_cap *uk_blkdev_capabilities(
+               struct uk_blkdev *blkdev)
+{
+       UK_ASSERT(blkdev);
+       UK_ASSERT(blkdev->_data->state >= UK_BLKDEV_RUNNING);
+
+       return &blkdev->capabilities;
+}
+
+#define uk_blkdev_ssize(blkdev) \
+       (uk_blkdev_capabilities(blkdev)->ssize)
+
+#define uk_blkdev_max_sec_per_req(blkdev) \
+       (uk_blkdev_capabilities(blkdev)->max_sectors_per_req)
+
+#define uk_blkdev_mode(blkdev) \
+       (uk_blkdev_capabilities(blkdev)->mode)
+
+#define uk_blkdev_sectors(blkdev) \
+       (uk_blkdev_capabilities(blkdev)->sectors)
+
+#define uk_blkdev_ioalign(blkdev) \
+       (uk_blkdev_capabilities(blkdev)->ioalign)
+/**
+ * Enable interrupts for a queue.
+ *
+ * @param dev
+ *     The Unikraft Block Device in running state.
+ * @param queue_id
+ *     The index of the queue to set up.
+ *     The value must be in the range [0, nb_queue - 1] previously supplied
+ *     to uk_blkdev_configure().
+ * @return
+ *     - (0): Success, interrupts enabled.
+ *     - (-ENOTSUP): Driver does not support interrupts.
+ */
+static inline int uk_blkdev_queue_intr_enable(struct uk_blkdev *dev,
+               uint16_t queue_id)
+{
+       UK_ASSERT(dev);
+       UK_ASSERT(dev->dev_ops);
+       UK_ASSERT(dev->_data);
+       UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+       UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
+
+       if (unlikely(!dev->dev_ops->queue_intr_enable))
+               return -ENOTSUP;
+
+       return dev->dev_ops->queue_intr_enable(dev, dev->_queue[queue_id]);
+}
+
+/**
+ * Disable interrupts for a queue.
+ *
+ * @param dev
+ *     The Unikraft Block Device in running state.
+ * @param queue_id
+ *     The index of the queue to set up.
+ *     The value must be in the range [0, nb_queue - 1] previously supplied
+ *     to uk_blkdev_configure().
+ * @return
+ *     - (0): Success, interrupts disabled.
+ *     - (-ENOTSUP): Driver does not support interrupts.
+ */
+static inline int uk_blkdev_queue_intr_disable(struct uk_blkdev *dev,
+               uint16_t queue_id)
+{
+       UK_ASSERT(dev);
+       UK_ASSERT(dev->dev_ops);
+       UK_ASSERT(dev->_data);
+       UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+       UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
+
+       if (unlikely(!dev->dev_ops->queue_intr_disable))
+               return -ENOTSUP;
+
+       return dev->dev_ops->queue_intr_disable(dev, dev->_queue[queue_id]);
+}
+
+/**
+ * Make an aio request to the device
+ *
+ * @param dev
+ *     The Unikraft Block Device
+ * @param queue_id
+ *     The index of the receive queue to receive from.
+ *     The value must be in the range [0, nb_queue - 1] previously supplied
+ *     to uk_blkdev_configure().
+ * @param req
+ *     Request structure
+ * @return
+ *     - (>=0): Positive value with status flags
+ *             - UK_BLKDEV_STATUS_SUCCESS: `req` was successfully put to the
+ *             queue.
+ *             - UK_BLKDEV_STATUS_MORE: Indicates there is still at least
+ *             one descriptor available for a subsequent transmission.
+ *             If the flag is unset means that the queue is full.
+ *             This may only be set together with UK_BLKDEV_STATUS_SUCCESS.
+ *     - (<0): Negative value with error code from driver, no request was sent.
+ */
+int uk_blkdev_queue_submit_one(struct uk_blkdev *dev, uint16_t queue_id,
+               struct uk_blkreq *req);
+
+/**
+ * Tests for status flags returned by `uk_blkdev_submit_one`
+ * When the function returned an error code or one of the selected flags is
+ * unset, this macro returns False.
+ *
+ * @param status
+ *     Return status (int)
+ * @param flag
+ *     Flag(s) to test
+ * @return
+ *     - (True):  All flags are set and status is not negative
+ *     - (False): At least one flag is not set or status is negative
+ */
+#define uk_blkdev_status_test_set(status, flag)                        \
+       (((int)(status) & ((int)(flag) | INT_MIN)) == (flag))
+
+/**
+ * Tests for unset status flags returned by `uk_blkdev_submit_one`
+ * When the function returned an error code or one of the
+ * selected flags is set, this macro returns False.
+ *
+ * @param status
+ *     Return status (int)
+ * @param flag
+ *     Flag(s) to test
+ * @return
+ *     - (True):  Flags are not set and status is not negative
+ *     - (False): At least one flag is set or status is negative
+ */
+#define uk_blkdev_status_test_unset(status, flag)                      \
+       (((int)(status) & ((int)(flag) | INT_MIN)) == (0x0))
+
+/**
+ * Tests if the return status of `uk_blkdev_submit_one`
+ * indicates a successful operation.
+ *
+ * @param status
+ *     Return status (int)
+ * @return
+ *     - (True):  Operation was successful
+ *     - (False): Operation was unsuccessful or error happened
+ */
+#define uk_blkdev_status_successful(status)                    \
+       uk_blkdev_status_test_set((status), UK_BLKDEV_STATUS_SUCCESS)
+
+/**
+ * Tests if the return status of `uk_blkdev_submit_one`
+ * indicates that the operation should be retried/
+ *
+ * @param status
+ *     Return status (int)
+ * @return
+ *     - (True):  Operation should be retried
+ *     - (False): Operation was successful or error happened
+ */
+#define uk_blkdev_status_notready(status)                              \
+       uk_blkdev_status_test_unset((status), UK_BLKDEV_STATUS_SUCCESS)
+
+/**
+ * Tests if the return status of `uk_blkdev_submit_one`
+ * indicates that the last operation can be successfully repeated again.
+ *
+ * @param status
+ *     Return status (int)
+ * @return
+ *     - (True):  Flag UK_BLKDEV_STATUS_MORE is set
+ *     - (False): Operation was successful or error happened
+ */
+#define uk_blkdev_status_more(status)                                  \
+       uk_blkdev_status_test_set((status), (UK_BLKDEV_STATUS_SUCCESS   \
+                                            | UK_BLKDEV_STATUS_MORE))
+
+/**
+ * Get responses from the queue
+ *
+ * @param dev
+ *     The Unikraft Block Device
+ * @param queue_id
+ *     queue id
+ * @return
+ *     - 0: Success
+ *     - (<0): on error returned by driver
+ */
+int uk_blkdev_queue_finish_reqs(struct uk_blkdev *dev, uint16_t queue_id);
+
  #ifdef __cplusplus
  }
  #endif
diff --git a/lib/ukblkdev/include/uk/blkdev_core.h 
b/lib/ukblkdev/include/uk/blkdev_core.h
index c4c458ab..13cd44ba 100644
--- a/lib/ukblkdev/include/uk/blkdev_core.h
+++ b/lib/ukblkdev/include/uk/blkdev_core.h
@@ -38,6 +38,7 @@
#include <uk/list.h>
  #include <uk/config.h>
+#include <uk/blkreq.h>
  #if defined(CONFIG_LIBUKBLKDEV_DISPATCHERTHREADS)
  #include <uk/sched.h>
  #include <uk/semaphore.h>
@@ -121,6 +122,9 @@ struct uk_blkdev_queue;
   *   The queue on the Unikraft block device on which the event happened.
   * @param argp
   *   Extra argument that can be defined on callback registration.
+ *
+ * Note: This should call dev->finish_reqs function in order to process the
+ *   received responses.
   */
  typedef void (*uk_blkdev_queue_event_t)(struct uk_blkdev *dev,
                uint16_t queue_id, void *argp);
@@ -163,12 +167,64 @@ typedef struct uk_blkdev_queue * 
(*uk_blkdev_queue_configure_t)(
  /** Driver callback type to start a configured Unikraft block device. */
  typedef int (*uk_blkdev_start_t)(struct uk_blkdev *dev);
+/**
+ * Driver callback type to enable interrupts
+ * for a queue on Unikraft block device.
+ **/
+typedef int (*uk_blkdev_queue_intr_enable_t)(struct uk_blkdev *dev,
+               struct uk_blkdev_queue *queue);
+
+/**
+ * Driver callback type to disable interrupts
+ *     for a queue on Unikraft block device.
+ **/
+typedef int (*uk_blkdev_queue_intr_disable_t)(struct uk_blkdev *dev,
+               struct uk_blkdev_queue *queue);
+/**
+ * Status code flags returned queue_submit_one function
+ */
+/** Successful operation. */
+#define UK_BLKDEV_STATUS_SUCCESS  (0x1)
+/**
+ * More room available for operation (e.g., still space on queue for sending
+ * a request.
+ */
+#define UK_BLKDEV_STATUS_MORE     (0x2)
+
+/** Driver callback type to submit a request to Unikraft block device. */
+typedef int (*uk_blkdev_queue_submit_one_t)(struct uk_blkdev *dev,
+               struct uk_blkdev_queue *queue, struct uk_blkreq *req);
+/**
+ * Driver callback type to finish
+ * a bunch of requests to Unikraft block device.
+ **/
+typedef int (*uk_blkdev_queue_finish_reqs_t)(struct uk_blkdev *dev,
+               struct uk_blkdev_queue *queue);
+
  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_queue_intr_enable_t                   queue_intr_enable;
+       uk_blkdev_queue_intr_disable_t                  queue_intr_disable;
+};
+
+/**
+ * Device info
+ */
+struct uk_blkdev_cap {
+       /* Number of sectors */
+       __sector sectors;
+       /* Sector size */
+       size_t ssize;
+       /* Access mode */

I will `#include <fcntl.h>` to this header and change the comment to
  /* Access mode (O_RDONLY, O_RDWR, O_WRONLY) */
while upstreaming. Then it should be clear what this field is about.

+       int mode;
+       /* Max nb of supported sectors for an op */
+       __sector max_sectors_per_req;
+       /* Alignment (number of bytes) for data used in future requests */
+       uint16_t ioalign;
  };
/**
@@ -216,8 +272,14 @@ struct uk_blkdev_data {
  };
struct uk_blkdev {
+       /* Pointer to submit request function */
+       uk_blkdev_queue_submit_one_t submit_one;
+       /* Pointer to handle_responses function */
+       uk_blkdev_queue_finish_reqs_t finish_reqs;
        /* Pointer to API-internal state data. */
        struct uk_blkdev_data *_data;
+       /* Capabilities. */
+       struct uk_blkdev_cap capabilities;
        /* Functions callbacks by driver. */
        const struct uk_blkdev_ops *dev_ops;
        /* Pointers to queues (API-private) */
diff --git a/lib/ukblkdev/include/uk/blkdev_driver.h 
b/lib/ukblkdev/include/uk/blkdev_driver.h
index 5dc9657d..534542a1 100644
--- a/lib/ukblkdev/include/uk/blkdev_driver.h
+++ b/lib/ukblkdev/include/uk/blkdev_driver.h
@@ -69,6 +69,34 @@ extern "C" {
  int uk_blkdev_drv_register(struct uk_blkdev *dev, struct uk_alloc *a,
                const char *drv_name);
+/**
+ * Forwards a queue event to the API user
+ * Can (and should) be called from device interrupt context
+ *
+ * @param dev
+ *     Unikraft block device to which the event relates to
+ * @param queue_id
+ *     receive queue ID to which the event relates to
+ */
+static inline void uk_blkdev_drv_queue_event(struct uk_blkdev *dev,
+               uint16_t queue_id)
+{
+       struct uk_blkdev_event_handler *queue_handler;
+
+       UK_ASSERT(dev);
+       UK_ASSERT(dev->_data);
+       UK_ASSERT(queue_id < CONFIG_LIBUKBLKDEV_MAXNBQUEUES);
+
+       queue_handler = &dev->_data->queue_handler[queue_id];
+
+#if CONFIG_LIBUKBLKDEV_DISPATCHERTHREADS
+       uk_semaphore_up(&queue_handler->events);
+#else
+       if (queue_handler->callback)
+               queue_handler->callback(dev, queue_id, queue_handler->cookie);
+#endif
+}
+
  #ifdef __cplusplus
  }
  #endif
diff --git a/lib/ukblkdev/include/uk/blkreq.h b/lib/ukblkdev/include/uk/blkreq.h
new file mode 100644
index 00000000..aa2cf9c4
--- /dev/null
+++ b/lib/ukblkdev/include/uk/blkreq.h
@@ -0,0 +1,112 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Roxana Nicolescu  <nicolescu.roxana1996@xxxxxxxxx>
+ *
+ * Copyright (c) 2019, University Politehnica of Bucharest
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the copyright holder nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
+ */
+#ifndef UK_BLKREQ_H_
+#define UK_BLKREQ_H_
+
+#include <uk/refcount.h>
+
+/**
+ * Unikraft block API request declaration.
+ *
+ * This header contains all the API data types used for requests operation.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define __sector size_t
+struct uk_blkreq;
+
+/**
+ *     Operation status
+ */
+enum uk_blkreq_state {
+       UK_BLKDEV_REQ_FINISHED = 0,
+       UK_BLKDEV_REQ_UNFINISHED
+};
+
+/**
+ * Supported operations
+ */
+enum uk_blkreq_op {
+       /* Read operation */
+       UK_BLKDEV_READ = 0,
+       /* Write operation */
+       UK_BLKDEV_WRITE,
+       /* Flush the volatile write cache */
+       UK_BLKDEV_FFLUSH = 4
+};
+
+/**
+ * Function type used for request callback after a response is processed.
+ *
+ * @param req
+ *     The request object on which the event is triggered
+ * @param cookie_callback
+ *     Optional parameter set by user at request submit.
+ */
+typedef void (*uk_blkreq_event_t)(struct uk_blkreq *req, void *cb_cookie);
+
+/**
+ * Used for sending a request to the device.
+ */
+struct uk_blkreq {
+       /* Input members */
+       /* Operation type */
+       enum uk_blkreq_op                       operation;
+       /* Start Sector from where the op begin */
+       __sector                                start_sector;
+       /* Size in number of sectors */
+       __sector                                nb_sectors;
+       /* Request callback and its parameters */
+       uk_blkreq_event_t               cb;
+       void                                    *cb_cookie;
+       /* Pointer to data */
+       void                                    *aio_buf;

Now, you moved it too far ;-). In the end it is only regarding readability. While upstreaming, I will move aio_buf before cookie to group I/O paremeters together again.

+
+       /* Output members */
+       /* State of request: finished/unfinished*/
+       __atomic                                state;
+       /* Result status of operation */

I will also change this comment to
        /* Result status of operation (< 0 on errors) */
...while upstreaming.

+       int                                     result;
+
+};
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* UK_BLKREQ_H_ */


_______________________________________________
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®.