[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH 4/6] lib/ukblkdev: Request interface
Hi Simon,
Thank you! See my comments below.
Roxana On 29.05.19 10:33, 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 | 32 +++++
> lib/ukblkdev/exportsyms.uk | 10 ++
> lib/ukblkdev/include/uk/blkdev.h | 200 ++++++++++++++++++++++++++++++++
> lib/ukblkdev/include/uk/blkdev_core.h | 63 ++++++++++
> lib/ukblkdev/include/uk/blkdev_driver.h | 28 +++++
> lib/ukblkdev/include/uk/blkreq.h | 110 ++++++++++++++++++
> 6 files changed, 443 insertions(+)
> create mode 100644 lib/ukblkdev/include/uk/blkreq.h
>
> diff --git a/lib/ukblkdev/blkdev.c b/lib/ukblkdev/blkdev.c
> index 6b82cf8d..3c58061b 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)
> @@ -371,3 +377,29 @@ 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_blkdev_request *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(req != NULL);
UK_ASSERT(!PTRISERR(dev->_queue[queue_id]));
> +
> + return dev->submit_one(dev, queue_id, req);
Don't you want to hand-over the queue reference instead? This way queue
ids are something that concerns a driver less. We did a similar thing
with libuknetdev. Our intention was that we did not want that each
driver implements an own lookup function, just to get its private queue
struct. It also kind of forces the driver implementation to separate
queue operations from operating the global device state. This is
something useful for SMP and preempted scheduling. later.
return dev->submit_one(dev, dev->_queue[queue_id], req);
I changed all the functions as you suggested in v2.
> +}
> +
> +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, queue_id);
I would handover the reference here, too:
return dev->finish_reqs(dev, dev->_queue[queue_id]);
> +}
> diff --git a/lib/ukblkdev/exportsyms.uk b/lib/ukblkdev/exportsyms.uk
> index 077994f0..5888be69 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_nb_sectors_per_req
maybe better to shroten the name of this one?: uk_blkdev_max_sec_per_req()
> +uk_blkdev_mode
What is the "access mode"? See my note at the capabilities struct.
> +uk_blkdev_sectors
> +uk_blkdev_align
call it uk_blkdev_ioalign, see also my comment later in this patch.
> +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 d32f62fc..db6d3b80 100644
> --- a/lib/ukblkdev/include/uk/blkdev.h
> +++ b/lib/ukblkdev/include/uk/blkdev.h
> @@ -221,6 +221,206 @@ int uk_blkdev_queue_configure(struct uk_blkdev *dev,
> */
> 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
Add a note that the device has to be "running" before you can get this
information.
> + *
> + * @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_nb_sectors_per_req(bldev) \
> + (uk_blkdev_capabilities(blkdev)->max_sectors_per_req)
> +
> +#define uk_blkdev_mode(blkdev) \
> + (uk_blkdev_capabilities(blkdev)->mode)
One thing: What is this mode? See my comments when we speak about the
capabilities struct in this file.
> +
> +#define uk_blkdev_sectors(blkdev) \
> + (uk_blkdev_capabilities(blkdev)->sectors)
> +
> +#define uk_blkdev_align(blkdev) \
> + (uk_blkdev_capabilities(blkdev)->align)
Call this one also 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, queue_id);
Same comment as with uk_blkdev_queue_submit_one():
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, queue_id);
Same comment here regarding the queue id (see intr_enable).
> +}
> +
> +/**
> + * 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_blkdev_request *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))
> +
INT_MIN requires the header <limits.h> to be included. Can you include
it in this header file?
> +/**
> + * 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 364c05d7..309b7a7f 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>
> @@ -119,6 +120,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);
> @@ -161,15 +165,68 @@ typedef struct uk_blkdev_queue * (*uk_blkdev_queue_configure_t)(
> uint16_t nb_desc,
> const struct uk_blkdev_queue_conf *queue_conf);
>
> +/**
> + * 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,
> + uint16_t queue_id); > +
I would use `struct uk_blkdev_queue *queue` instead
of `uint16_t queue_id` (see my comment ahead). Please also group this
prototype definition together with the intr_disable defintion.
> /** Driver callback type to start a configured Unikraft block device. */
> typedef int (*uk_blkdev_start_t)(struct uk_blkdev *dev);
>
> +/**
> + * 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,
> + uint16_t queue_id);
Same comment: `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,
> + uint16_t queue_id,
Same comment: `struct uk_blkdev_queue *queue`
> + struct uk_blkdev_request *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,
> + uint16_t queue_id);
Same comment: `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_queue_intr_enable_t queue_intr_enable;
> uk_blkdev_start_t dev_start;
> + uk_blkdev_queue_intr_disable_t queue_intr_disable;
> +};
> +
> +/**
> + * Device info
> + */
> +struct uk_blkdev_cap {
> + /* Number of sectors */
> + __sector sectors;
> + /* Sector size */
> + __sector ssize;
It does not make sense to define the size of a single sector with a unit
"in number of sectors" (`__sector`). You can take `size_t` or `uint16_t`
instead.
> + /* Access mode */
> + int mode;
What is the access mode (is it for instance read-only etc.)? Is the
driver filling this out and it is read-only for the API user?
I am asking because I could not find any defintions (e..g, enum or
constants) that describe what the access mode means.
The access mode follows the defines from fcntl.h (nolibc): O_RDONLY or O_RDWR.
> + /* Max nb of supported sectors for an op */
> + __sector max_sectors_per_req;
> + /* Alignment for data used in future read/write requests */
> + uint16_t align;
I would call this field `ioalign` to point out that this alignment has
to do with alignment of I/O operations.
You can optionally comment that this alignment means in number of bytes.
If `ioalign` is `1`, no alignment is set, right?
Right.
> };
>
> /**
> @@ -215,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 b72f52ab..37b5a3e8 100644
> --- a/lib/ukblkdev/include/uk/blkdev_driver.h
> +++ b/lib/ukblkdev/include/uk/blkdev_driver.h
> @@ -70,6 +70,34 @@ 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..85c25587
> --- /dev/null
> +++ b/lib/ukblkdev/include/uk/blkreq.h
> @@ -0,0 +1,110 @@
> +/* 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_
> +
> +/**
> + * 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_blkdev_request;
I prefer naming it `uk_blkreq` instead. This would be a similar
shortening of the struct name like we did in libuknetdev with `struct
uk_netbuf`. It should improve readability.
> +
> +/**
> + * Operation status
> + */
> +enum uk_blkdev_req_state {
> + UK_BLKDEV_REQ_FINISHED = 0,
> + UK_BLKDEV_REQ_UNFINISHED
> +};
Simplify the name with `uk_blkreq_state`
> +
> +/**
> + * Supported operations
> + */
> +enum uk_blkdev_op {
> + /* Read operation */
> + UK_BLKDEV_READ = 0,
> + /* Write operation */
> + UK_BLKDEV_WRITE,
> + /* Flush the volatile write cache */
> + UK_BLKDEV_FFLUSH = 4
> +};
Simplify the name with `uk_blkreq_op`
Btw, is it possible that drivers can write and flush at the same time or
do you expect them to do two independent request. In the latter case the
enum is fine. Otherwise I would define Macros that can be or'ed.
No, it is not possible to write and flush at the same time. These are two different (separate) operations. That's why I use an enum for that.
> +
> +/**
> + * 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
> + */
> +typedef int (*uk_blkdev_request_event_t)(struct uk_blkdev_request *req,
> + void *cookie_callback);
I would call `cookie_callback` it either `cb_cookie`, `cb_arg` or just
`cookie`. Please mention that the argument is the one that was given by
the user on the request setup/submission.
I would also name the function pointer data type `uk_blkreq_event_t`
instead. It should be also a `void` function. I think it makes the API
too complicated if the user-provided function can return error codes.
> +
> +/**
> + * Used for sending a request to the device.
> + */
> +struct uk_blkdev_request {
> + /* Input members */
> + /* Operation type */
> + enum uk_blkdev_op operation;
With this enum it looks like that you can't WRITE and FFLUSH with the
same request, is this intended?
Yes, see my previous comment.
> + /* Start Sector from where the op begin */
> + size_t start_sector;
Since the start sector is a sector address, you should use `__sector` as
data type instead of `size_t`. I would also call the field also just
`start`.
> + /* Pointer to data */
> + void *aio_buf;
> + /* Size in number of sectors */
> + __sector nb_sectors;
To group the arguments better, please move nb_sectors before `aio_buf`.
Actually, I chose to put the aio_buf before the output members, since for read operations it is used for input and output as well (back end will put data at this address).
> + /* Request callback and its parameters */
> + uk_blkdev_request_event_t cb;
> + void *cookie_callback;
I would call it either `cb_cookie`, `cb_arg` or just `cookie`.
> +
> + /* Output members */
> + /* State of request: finished/unfinished*/
> + __atomic state;
It probably makes sense to provide an init function (static inline) or
macro for requests. This one can be used to initialize the output
parameters. This would probably simplify the usage of the API and we can
extend it later without breaking existing code too much, for instance
with scatter-gather lists.
Something along these lines:
static inline void uk_blkreq_init(struct uk_blkreq *r,
enum uk_blkreq_op op,
__sector start, __sector nb_sectors,
void *aio_buf,
uk_blkreq_event_t cb,
void *cb_arg) { [...] }
In case you introduce it, please use it within your synchronous API.
I would also introduce a small helper macro that can be used to poll the
state:
#define uk_blkreq_is_done(req) \
[..the atomic operation for testing..]
...to be used like:
while (!uk_blkreq_is_done(req)) {
/* poll the device queue */
[...]
}
Good idea. This can be used in the sync function as well.
> + /* Result status of operation */
> + uint8_t result_status;
I would name the result field just `result`. Are you sure you want to
have an unsigned int of 8 bits? I would have expected `int`. Errors are
negative and success is zero. How is it actually defined? Maybe a
description is needed here. Actually, in both drivers (Xen and Virtio) the error status are positive. I will change the sign of it in v2.
> +};
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* UK_BLKREQ_H_ */
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
