[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH v4 11/11] lib/uknetdev: Packet reception and transmission interfaces
Hello Simon,
This patch looks good. Please find the some minor comments inlines
* There are minor changes grammatical suggestion in the API description
which can be fixed while we pushing them.
* The suggested change to move the interrupt enable/disable would be a
nice to have.
On 10/10/2018 02:00 PM, Simon Kuenzer wrote:
From: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
Introduce interfaces for single packet transmission and
reception. Both interfaces are designed to support asynchronous
zero-copy operation with netbufs. Receiving can be done with
interrupts or with polling. Transmission supports only polling mode
for now.
Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
Signed-off-by: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
---
lib/uknetdev/exportsyms.uk | 2 +
lib/uknetdev/include/uk/netdev.h | 130 ++++++++++++++++++++++++++++++++
lib/uknetdev/include/uk/netdev_core.h | 36 ++++++++-
lib/uknetdev/include/uk/netdev_driver.h | 28 +++++++
lib/uknetdev/netdev.c | 33 ++++++++
5 files changed, 227 insertions(+), 2 deletions(-)
diff --git a/lib/uknetdev/exportsyms.uk b/lib/uknetdev/exportsyms.uk
index 9e264bd..d6f6fcf 100644
--- a/lib/uknetdev/exportsyms.uk
+++ b/lib/uknetdev/exportsyms.uk
@@ -28,3 +28,5 @@ uk_netdev_promiscuous_get
uk_netdev_promiscuous_set
uk_netdev_mtu_get
uk_netdev_mtu_set
+uk_netdev_rxq_intr_enable
+uk_netdev_rxq_intr_disable
diff --git a/lib/uknetdev/include/uk/netdev.h b/lib/uknetdev/include/uk/netdev.h
index e527534..11dd7df 100644
--- a/lib/uknetdev/include/uk/netdev.h
+++ b/lib/uknetdev/include/uk/netdev.h
@@ -362,6 +362,136 @@ int uk_netdev_mtu_get(struct uk_netdev *dev);
*/
int uk_netdev_mtu_set(struct uk_netdev *dev, uint16_t mtu);
+/**
+ * Enable interrupts for an RX queue.
+ *
+ * @param dev
+ * The Unikraft Network Device in running state.
+ * @param queue_id
+ * The index of the receive queue to set up.
+ * The value must be in the range [0, nb_rx_queue - 1] previously supplied
+ * to uk_netdev_configure().
+ * @return
+ * - (0): Success, interrupts enabled.
+ * - (1): More packets are left on the queue, interrupts are NOT enabled yet.
+ * Interrupts will be automatically enabled when all received packets
+ * on the queue are consumed and the receive queue is empty.
+ * - (-ENOTSUP): Driver does not support interrupts.
+ */
+int uk_netdev_rxq_intr_enable(struct uk_netdev *dev,
+ uint16_t rx_queue_id);
+
+/**
+ * Disable interrupts for an RX queue.
+ *
+ * @param dev
+ * The Unikraft Network Device in running state.
+ * @param queue_id
+ * The index of the receive queue to set up.
+ * The value must be in the range [0, nb_tx_queue - 1] previously supplied
+ * to uk_netdev_configure().
+ * @return
+ * - (0): Success, interrupts disabled.
+ * - (-ENOTSUP): Driver does not support interrupts.
+ */
+int uk_netdev_rxq_intr_disable(struct uk_netdev *dev,
+ uint16_t rx_queue_id);
+
+/**
+ * Receive one packet and re-program used receive descriptor
+ * Please note that before any packet can be received, the receive queue
+ * has to be filled up with empty netbufs (see fillup parameter).
+ *
+ * @param dev
+ * The Unikraft Network Device.
+ * @param queue_id
+ * The index of the receive queue to receive from.
+ * The value must be in the range [0, nb_rx_queue - 1] previously supplied
+ * to uk_netdev_configure().
+ * @param pkt
+ * Reference to netbuf pointer which will be point to the received packet
+ * after the function call. Can be NULL if function is used to program
+ * receive descriptors only.
+ * @param fillup
+ * Array of netbufs that should be used to program used descriptors again.
+ * Each of the netbuf should be freshly allocated/initialized and not part
+ * of any chain.
+ * `fillup` can be `NULL` but without re-programming of used descriptors no
+ * new packets can be received at some point.
+ * @param fillup_count
+ * Length of `fillup` array. After the function call, `fillup_count` returns
+ * the number of left and unused netbufs on the array. `fillup_count` has to
+ * to 0 if `fillup` is `NULL`.
+ * @return
+ * - (0): No packet available or `pkt` was set to NULL,
+ * check `fillup_count` for used `fillup` netbufs
+ * - (1): `pkt` points to received netbuf,
+ * check `fillup_count` for used `fillup` netbufs
+ * - (2): `pkt` points to received netbuf but more received packets are
+ * available on the receive queue. When interrupts are used, they are
Shouldn't it be subsequent calls?
+ * disabled until 1 is returned on a subsequent call
+ * check `fillup_count` for used `fillup` netbufs
+ * - (<0): Error code from driver
+ */
+static inline int uk_netdev_rx_one(struct uk_netdev *dev, uint16_t queue_id,
+ struct uk_netbuf **pkt,
+ struct uk_netbuf *fillup[],
+ uint16_t *fillup_count)
+{
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->rx_one);
Should we not compare against nb_rx_queue?
+ UK_ASSERT(queue_id < CONFIG_LIBUKNETDEV_MAXNBQUEUES);
+ UK_ASSERT(dev->_data->state == UK_NETDEV_RUNNING);
+ UK_ASSERT(!PTRISERR(dev->_rx_queue[queue_id]));
+ UK_ASSERT((!fillup && fillup_count) || fillup);
+
+ return dev->rx_one(dev, dev->_rx_queue[queue_id], pkt,
+ fillup, fillup_count);
+}
+
+/**
+ * Shortcut for only filling up a receive queue with empty netbufs
+ */
+#define uk_netdev_rx_fillup(dev, queue_id, fillup, fillup_count) \
+ uk_netdev_rx_one((dev), (queue_id), NULL, (fillup), (fillup_count))
+
+/**
+ * Transmit one packet
+ *
+ * @param dev
+ * The Unikraft Network Device.
+ * @param queue_id
+ * The index of the receive queue to receive from.
+ * The value must be in the range [0, nb_tx_queue - 1] previously supplied
+ * to uk_netdev_configure().
+ * @param pkt
+ * Reference to netbuf to sent. Packet is free'd by the driver after sending
+ * was successfully finished by the device.
+ * Please note that some drivers may require available headroom on the netbuf
+ * for doing a transmission - inspect `nb_encap` with uk_netdev_info_get().
+ * `pkt` has never to be `NULL`.
+ * @return
+ * - (0): No space left on transmit queue, `pkt` is not sent
+ * - (1): `pkt` was successfully put to the transmit queue,
+ * queue is currently full
+ * - (2): `pkt` was successfully put to the transmit queue,
+ * there is still at least one descriptor available for a
+ * subsequent transmission
+ * - (<0): Error code from driver, `pkt` is not sent
+ */
+static inline int uk_netdev_tx_one(struct uk_netdev *dev, uint16_t queue_id,
+ struct uk_netbuf *pkt)
+{
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->tx_one);
Should we not compare against nb_tx_queue?
+ UK_ASSERT(queue_id < CONFIG_LIBUKNETDEV_MAXNBQUEUES);
+ UK_ASSERT(dev->_data->state == UK_NETDEV_RUNNING);
+ UK_ASSERT(!PTRISERR(dev->_tx_queue[queue_id]));
+ UK_ASSERT(pkt);
+
+ return dev->tx_one(dev, dev->_tx_queue[queue_id], pkt);
+}
+
#ifdef __cplusplus
}
#endif
diff --git a/lib/uknetdev/include/uk/netdev_core.h
b/lib/uknetdev/include/uk/netdev_core.h
index f78f992..479007e 100644
--- a/lib/uknetdev/include/uk/netdev_core.h
+++ b/lib/uknetdev/include/uk/netdev_core.h
@@ -253,10 +253,34 @@ typedef uint16_t (*uk_netdev_mtu_get_t)(struct uk_netdev
*dev);
/** Driver callback type to set the MTU */
typedef int (*uk_netdev_mtu_set_t)(struct uk_netdev *dev, uint16_t mtu);
+/** Driver callback type to enable interrupts of a RX queue */
+typedef int (*uk_netdev_rxq_intr_enable_t)(struct uk_netdev *dev,
+ struct uk_netdev_rx_queue *queue);
+
+/** Driver callback type to disable interrupts of a RX queue */
+typedef int (*uk_netdev_rxq_intr_disable_t)(struct uk_netdev *dev,
+ struct uk_netdev_rx_queue *queue);
+
+/** Driver callback type to retrieve one packet from a RX queue. */
+typedef int (*uk_netdev_rx_one_t)(struct uk_netdev *dev,
+ struct uk_netdev_rx_queue *queue,
+ struct uk_netbuf **pkt,
+ struct uk_netbuf *fillup[],
+ uint16_t *fillup_count);
+
+/** Driver callback type to submit one packet to a TX queue. */
+typedef int (*uk_netdev_tx_one_t)(struct uk_netdev *dev,
+ struct uk_netdev_tx_queue *queue,
+ struct uk_netbuf *pkt);
+
/**
* A structure containing the functions exported by a driver.
*/
struct uk_netdev_ops {
+ /** RX queue interrupts. */
+ uk_netdev_rxq_intr_enable_t rxq_intr_enable; /* optional */
+ uk_netdev_rxq_intr_disable_t rxq_intr_disable; /* optional */
+
/** Set/Get hardware address. */
uk_netdev_hwaddr_get_t hwaddr_get; /* recommended */
uk_netdev_hwaddr_set_t hwaddr_set; /* optional */
@@ -318,10 +342,18 @@ struct uk_netdev_data {
* NETDEV
* A structure used to interact with a network device.
*
- * Function callbacks (ops) are registered by the driver before
- * registering the netdev. They change during device life time.
+ * Function callbacks (tx_one, rx_one, ops) are registered by the driver before
+ * registering the netdev. They change during device life time. Packet RX/TX
+ * functions are added directly to this structure for performance reasons.
+ * It prevents another indirection to ops.
*/
struct uk_netdev {
+ /** Packet transmission. */
+ uk_netdev_tx_one_t tx_one; /* by driver */
+
+ /** Packet reception. */
+ uk_netdev_rx_one_t rx_one; /* by driver */
+
/** Pointer to API-internal state data. */
struct uk_netdev_data *_data;
diff --git a/lib/uknetdev/include/uk/netdev_driver.h b/lib/uknetdev/include/uk/netdev_driver.h
index ac8ce61..512b74b 100644
--- a/lib/uknetdev/include/uk/netdev_driver.h
+++ b/lib/uknetdev/include/uk/netdev_driver.h
@@ -67,6 +67,34 @@ extern "C" {
int uk_netdev_drv_register(struct uk_netdev *dev, struct uk_alloc *a,
const char *drv_name);
+/**
+ * Forwards an RX queue event to the API user
+ * Can (and should) be called from device interrupt context
+ *
+ * @param dev
+ * Unikraft network device to which the event relates to
+ * @param queue_id
+ * receive queue ID to which the event relates to
+ */
+static inline void uk_netdev_drv_rx_event(struct uk_netdev *dev,
+ uint16_t queue_id)
+{
+ struct uk_netdev_event_handler *rxq_handler;
+
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->_data);
queue_id should be compared against nb_rx_queue?
+ UK_ASSERT(queue_id < CONFIG_LIBUKNETDEV_MAXNBQUEUES);
+
+ rxq_handler = &dev->_data->rxq_handler[queue_id];
+
+#ifdef CONFIG_LIBUKNETDEV_DISPATCHERTHREADS
+ uk_semaphore_up(&rxq_handler->events);
+#else
+ if (rxq_handler->callback)
+ rxq_handler->callback(dev, queue_id, rxq_handler->cookie);
+#endif
+}
+
#ifdef __cplusplus
}
#endif
diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
index 6277354..6ee0fba 100644
--- a/lib/uknetdev/netdev.c
+++ b/lib/uknetdev/netdev.c
@@ -80,6 +80,11 @@ int uk_netdev_drv_register(struct uk_netdev *dev, struct
uk_alloc *a,
UK_ASSERT(dev->ops->start);
UK_ASSERT(dev->ops->promiscuous_get);
UK_ASSERT(dev->ops->mtu_get);
+ UK_ASSERT((dev->ops->rxq_intr_enable && dev->ops->rxq_intr_disable)
+ || (!dev->ops->rxq_intr_enable
+ && !dev->ops->rxq_intr_disable));
+ UK_ASSERT(dev->rx_one);
+ UK_ASSERT(dev->tx_one);
dev->_data = _alloc_data(a, netdev_count, drv_name);
if (!dev->_data)
@@ -519,3 +524,31 @@ int uk_netdev_mtu_set(struct uk_netdev *dev, uint16_t mtu)
return dev->ops->mtu_set(dev, mtu);
}
+
Maybe we could inline these functions in netdev.h?
+int uk_netdev_rxq_intr_enable(struct uk_netdev *dev, uint16_t queue_id)
+{
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->ops);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(dev->_data->state == UK_NETDEV_RUNNING);
+ UK_ASSERT(queue_id < CONFIG_LIBUKNETDEV_MAXNBQUEUES);
+ UK_ASSERT(!PTRISERR(dev->_rx_queue[queue_id]));
+
+ if (unlikely(!dev->ops->rxq_intr_enable))
+ return -ENOTSUP;
+ return dev->ops->rxq_intr_enable(dev, dev->_rx_queue[queue_id]);
+}
+
Maybe we could inline these functions in netdev.h?
+int uk_netdev_rxq_intr_disable(struct uk_netdev *dev, uint16_t queue_id)
+{
+ UK_ASSERT(dev);
+ UK_ASSERT(dev->ops);
+ UK_ASSERT(dev->_data);
+ UK_ASSERT(dev->_data->state == UK_NETDEV_RUNNING);
+ UK_ASSERT(queue_id < CONFIG_LIBUKNETDEV_MAXNBQUEUES);
+ UK_ASSERT(!PTRISERR(dev->_rx_queue[queue_id]));
+
+ if (unlikely(!dev->ops->rxq_intr_disable))
+ return -ENOTSUP;
+ return dev->ops->rxq_intr_disable(dev, dev->_rx_queue[queue_id]);
+}
Thanks & Regards
Sharan
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
