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/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.

>>   };
>>     /**
>> 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)
>