Re: [UNIKRAFT PATCH v4 01/12] plat/drivers/virtio: Fill MTU and queues alignments when retrieving information

[Costin Lupu <costin.lupu@xxxxxxxxx>]

On 8/20/20 6:47 PM, Simon Kuenzer wrote:
> Wouldn't this patch go independently to netfront? I do not see the
> relation...
> 

Yes, this can be pushed separately. It's in the series because it's a
longer story behind. Sharan reviewed its previous version and it would
make sense if he would review the current one as well.

> On 13.08.20 10:53, Costin Lupu wrote:
>> This patch sets the max MTU value when retrieving device information
>> and the
>> alignments used when allocating packets for each queue.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>>   plat/drivers/virtio/virtio_net.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/plat/drivers/virtio/virtio_net.c
>> b/plat/drivers/virtio/virtio_net.c
>> index b0663e60..0e748b8f 100644
>> --- a/plat/drivers/virtio/virtio_net.c
>> +++ b/plat/drivers/virtio/virtio_net.c
>> @@ -770,6 +770,7 @@ static int virtio_netdev_rxq_info_get(struct
>> uk_netdev *dev,
>>       rxq = &vndev->rxqs[queue_id];
>>       qinfo->nb_min = 1;
>>       qinfo->nb_max = rxq->max_nb_desc;
>> +    qinfo->nb_align = sizeof(void *); /* word size alignment */
> 
> @Costin, @Sharan: Does changing the alignment something good or bad to
> the virtio performance? I guess it does not harm. 0 meant that there is
> no alignment requirement for network packet buffers.
> 

Word size alignment is better than no alignment. Given that you want
some performance, I don't think you'd be happy with no alignment. Btw,
with the buddy allocator all unaligned allocations do return (at least)
word size aligned addresses.

>>       qinfo->nb_is_power_of_two = 1;
>>     exit:
>> @@ -797,6 +798,7 @@ static int virtio_netdev_txq_info_get(struct
>> uk_netdev *dev,
>>       txq = &vndev->txqs[queue_id];
>>       qinfo->nb_min = 1;
>>       qinfo->nb_max = txq->max_nb_desc;
>> +    qinfo->nb_align = sizeof(void *); /* word size alignment */
> 
> @Costin, @Sharan: ...same here.
> 
>>       qinfo->nb_is_power_of_two = 1;
>>     exit:
>> @@ -1048,6 +1050,7 @@ 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;
> 
> I agree, this should be announced.
> 
>>       dev_info->nb_encap_tx = sizeof(struct virtio_net_hdr_padded);
>>       dev_info->nb_encap_rx = sizeof(struct virtio_net_hdr_padded);
>>       dev_info->features = UK_FEATURE_RXQ_INTR_AVAILABLE;
>>
>