|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH 4/6] lib/ukblkdev: Request interface
On 29.05.19 10:33, Roxana Nicolescu wrote:
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); 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. 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. Same comment as with uk_blkdev_queue_submit_one(): return dev->dev_ops->queue_intr_enable(dev, dev->_queue[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> I would use `struct uk_blkdev_queue *queue` insteadof `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` 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. + /* 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? };/** 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. Simplify the name with `uk_blkreq_state` 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. + +/** + * 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. With this enum it looks like that you can't WRITE and FFLUSH with the same request, is this intended? + /* 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`. + /* 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 */
[...]
}
+ /* 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. +}; + +#ifdef __cplusplus +} +#endif + +#endif /* UK_BLKREQ_H_ */ _______________________________________________ Minios-devel mailing list Minios-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |