Re: [Minios-devel] [UNIKRAFT PATCH v3 02/12] lib/uknetdev: Add alignment size for packet buffers allocations

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Costin,

On 8/11/20 12:40 PM, Costin Lupu wrote:
> Hi Sharan,
>
> Please see inline.
>
> On 8/11/20 11:40 AM, Sharan Santhanam wrote:
> > Hello Costin,
> >
> > Thank you for the work. Please find the comments inline.
> >
> > Thanks & Regards
> > Sharan
> >
> > On 3/3/20 3:13 PM, Costin Lupu wrote:
> > > Packet buffers may need different alignments, depending on their driver
> > > requirements. For Xen netfront driver, packet buffers have to be page
> > > aligned
> > > because they are saved in pages shared between backend and frontend.
> > > For KVM
> > > virtio net driver, word size alignments suffice. Therefore, we need to
> > > save the
> > > alignment size in order to support multiple drivers.
> > >
> > > This patch also squeezes a small fix for setting the max_mtu when
> > > retrieving
> > > device information from the virtio net driver.
> > >
> > > Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> > > ---
> > >    lib/uknetdev/include/uk/netdev_core.h | 1 +
> > >    plat/drivers/virtio/virtio_net.c      | 2 ++
> > >    2 files changed, 3 insertions(+)
> > >
> > > diff --git a/lib/uknetdev/include/uk/netdev_core.h
> > > b/lib/uknetdev/include/uk/netdev_core.h
> > > index f073e101..268d54d7 100644
> > > --- a/lib/uknetdev/include/uk/netdev_core.h
> > > +++ b/lib/uknetdev/include/uk/netdev_core.h
> > > @@ -94,6 +94,7 @@ struct uk_netdev_info {
> > >        uint16_t max_mtu;   /**< Maximum supported MTU size. */
> > >        uint16_t nb_encap_tx;  /**< Number of bytes required as headroom
> > > for tx. */
> > >        uint16_t nb_encap_rx;  /**< Number of bytes required as headroom
> > > for rx. */
> > > +    uint16_t align;  /**< Alignment required for data address. */
> > Why do we need to introduce yet another align field. Each of the tx
> > queue and rx queue has a `uk_netdev_queue_info` which have the align
> > fields. Since packet buffers are more tied to the rxq and txq why dont
> > we use those align field.
> Indeed, I overlooked the nb_align field from the queue info. However, it
> isn't initialized or used anywhere in the code. Besides this, isn't
> alignment a property of the device rather than the queue? Are there
> network devices with different alignments for their queues? Maybe we
> should drop the field from the queue and use the one from device info
> instead.
>
> I needed the alignment for allocating buffers in lwip (please see
> function `netbuf_alloc_helper_init()` in lwip glue code). All the
> information needed for allocating buffers seems to be device specific,
> not queue specific. If we had used the nb_field of queues then we would
> have a buffer allocation helper for each queue.
The alignment property wasn't used so far because we did not have 
alignment requirement for the other driver. Virtio had requirement on 
the number of descriptors should be a power of two and i guess that 
would be set in the virtio drivers.
 I see your point on the necessity to have a uniform queue information 
for all the rx/tx queues of a netdevice. But we could get around the 
problem, by getting the queue info for a one queue setting it all the 
different instance of rx and tx queue while still maintaining the 
configuration granularity of a queue. The other option might be to move 
the `uk_netdev_queue_info` as a struct into the `netdev_info` with 2 
different fields as `rxq_info` and `txq_info` and fetch the information 
only once when configuring the respective queue. This also leads to the 
question on where we should use the tx_headroom and rx_headroom be 
defined. Since the scope of the patch series changes, we can split it as 
a separate patch series. For the current problem, I would use the 
`nb_align` field from `uk_netdev_queue_info` since the queue properties 
would remain either ways.
In the netdev API, both the rx_one and tx_one operation happens at the 
queue level and not on a device level since we select the queue on which 
send/receive the packet from the netdevice. The netdev API have taken 
the multiqueue in account. The receive part on LWIP happens also at the 
queue level as all the callback are set for a queue configuration, while 
the tx part is still the one were netdevice where we don't use the queue 
information yet.
`netbuf_alloc_helper_init` does not have any dependency to a netdevice 
except we pass the netdev_info to it and collapse the netdev_info to a 
flat structure for that interface which is a valid method for lwip to 
handle its memory allocation. A suggestion might be fetch the device and 
queue information as a part of the `netbuf_alloc_helper_init`. But the 
needs of LWIP may not cause a change to netdev as it is a specific use 
case. The other use case of running DPDK on Unikraft does not have these 
requirements. I would keep the netdev API flexible enough and handle the 
specific use case at a higher level than netdev.

Thanks & Regards

Sharan

> > >    };
> > >      /**
> > > diff --git a/plat/drivers/virtio/virtio_net.c
> > > b/plat/drivers/virtio/virtio_net.c
> > > index efc2cb71..9f1873c5 100644
> > > --- a/plat/drivers/virtio/virtio_net.c
> > > +++ b/plat/drivers/virtio/virtio_net.c
> > > @@ -1048,8 +1048,10 @@ static void virtio_net_info_get(struct
> > > uk_netdev *dev,
> > >          dev_info->max_rx_queues = vndev->max_vqueue_pairs;
> > >        dev_info->max_tx_queues = vndev->max_vqueue_pairs;
> > > +    dev_info->max_mtu = vndev->max_mtu;
> > >        dev_info->nb_encap_tx = sizeof(struct virtio_net_hdr_padded);
> > >        dev_info->nb_encap_rx = sizeof(struct virtio_net_hdr_padded);
> > > +    dev_info->align = sizeof(void *); /* word size alignment */
> > >    }
> > >      static int virtio_net_start(struct uk_netdev *n)