On Wed, Mar 01, 2017 at 06:14:21AM +0200, Michael S. Tsirkin wrote:
> > > > That said, if it's "16: 16" and if we use only 8 bits for batch, we
> > > > could still have another 8 bit for anything else, say the number of
> > > > desc for a single pkt. With that, the num_buffers of mergeable Rx
> > > > header could be replaced. More importantly, we could reduce a cache
> > > > line write if non offload are supported in mergeable Rx path.
> > >
> > > Why do you bother with mergeable Rx without offloads?
> >
> > Oh, my bad. I actually meant "without offloads __being used__". Just
> > assume the pkt size is 64B and no offloads are used. When mergeable
> > Rx is negotiated (which is the default case), num_buffers has to be
> > set 1. That means an extra cache line write. For the case of non
> > mergeable, the cache line write could be avoid by a trick like what
> > the following patch did:
> >
> >
> > http://dpdk.org/browse/dpdk/commit/?id=c9ea670c1dc7e3f111d8139f915082b60c9c1ffe
> >
> > It basically tries to avoid writing 0 if the value is already 0:
> > the case when no offloads are used.
> > So while writing this email, I was thinking maybe we could not set
> > num_buffers to 1 when there is only one desc (let it be 0 and let
> > num_buffers == 0 imply num_buffers = 1). I'm not quite sure we can
> > do that now, thus I checked the DPDK code and found it's Okay.
> >
> > 896 seg_num = header->num_buffers;
> > 897
> > 898 if (seg_num == 0)
> > 899 seg_num = 1;
> >
> >
> > I then also checked linux kernel code, and found it's not okay as
> > the code depends on the value being set correctly:
> >
> > ==> 365 u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> > 366 struct page *page = virt_to_head_page(buf);
> > 367 int offset = buf - page_address(page);
> > 368 unsigned int truesize = max(len,
> > mergeable_ctx_to_buf_truesize(ctx));
> > 369
> > 370 struct sk_buff *head_skb = page_to_skb(vi, rq, page,
> > offset, len,
> > 371 truesize);
> > 372 struct sk_buff *curr_skb = head_skb;
> > 373
> > 374 if (unlikely(!curr_skb))
> > 375 goto err_skb;
> > ==> 376 while (--num_buf) {
> >
> > That means, if we want to do that, it needs an extra feature flag
> > (either a global feature flag or a desc flag), something like
> > ALLOW_ZERO_NUM_BUFFERS. Or even, make it allowable in virtio 1.1
> > (virtio 0.95/1.0 won't benifit from it though).
> >
> > Does it make sense to you?
>
> Right and then we could use a descriptor flag "header is all 0s".
> For virtio 1.0 we could put these in the used ring instead.
Great.
--yliu
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
