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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 21.08.20 11:18, Costin Lupu wrote:
> 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.
Thanks for the explanation. I leave it up you and Sharan how to handle it.

> > 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.
In general, I agree and would do the same. I just wanted to confirm the 
fact since Sharan was working heavily on virtio-net performance. His 
changes need to go upstream still.
> > >        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;
Thanks,

Simon