Re: [Minios-devel] [UNIKRAFT PATCH v4 11/11] lib/uknetdev: Packet reception and transmission interfaces

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 11.10.18 17:52, Sharan Santhanam wrote:
> 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?
CONFIG_LIBUKNETDEV_MAXNBQUEUES is the upper limit from the API 
perspective. Something bigger would imply that we are accessing later 
the _rx_queue array out of bounds. Even if a driver would support more 
queues, libuknetdev is limiting it (uk_netdev_info_get() limits the 
returned value). For the case the queue id is smaller than the API 
supports, it was the intention that a driver is doing the check (which 
it probably anyway does). We did not want to store too many duplicated 
values within the API.
> > +    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?
Same argument here.

> > +    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?
Same here.

> > +    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?
Yes, we could. I will do it for the v5.

> > +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
Thanks for all your review and your comments. I will send a v5 soon.


Thanks,

Simon

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel