[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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.