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

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Hi Sharan,

Please see inline.

On 8/12/20 3:04 PM, Sharan Santhanam wrote:
> 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. 

Ok, so let me see if I got this right. Are you saying that I should get
the nb_align values only for the first rx queue and the first tx queue
and that would be enough?

> 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)
>