On Wed, Mar 01, 2017 at 03:02:29AM +0200, Michael S. Tsirkin wrote: > > > * Descriptor ring: > > > > > > Guest adds descriptors with unique index values and DESC_HW set in flags. > > > Host overwrites used descriptors with correct len, index, and DESC_HW > > > clear. Flags are always set/cleared last. > > > > May I know what's the index intended for? Back referencing a pkt buffer? > > Yes and generally identify which descriptor completed. Recall that > even though vhost net completes in order at the moment, > virtio rings serve devices (e.g. storage) that complete out of order. I see, and thanks. > > 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? > Make each buffer > MTU sized and it'll fit without merging. Linux used not to, it only > started doing this to save memory aggressively. I don't think > DPDK cares about this. > > > > > struct desc { > > __le64 addr; > > __le16 len; > > __le8 batch; > > __le8 num_buffers; > > __le16 index; > > __le16 flags; > > }; > > Interesting. How about a benchmark for these ideas? Sure, I would like to benchmark it. It won't take long to me. But currently, I was still focusing on analysising the performance behaviour of virtio 0.95/1.0 (when I get some time), to see what's not good for performance and what's can be improved. Besides that, as said, I often got interrupted. Moreoever, DPDK v17.05 code freeze deadline is coming. So it's just a remind that I may don't have time for it recently. Sorry for that. > > > * Device specific descriptor flags > > > We have a lot of unused space in the descriptor. This can be put to > > > good use by reserving some flag bits for device use. > > > For example, network device can set a bit to request > > > that header in the descriptor is suppressed > > > (in case it's all 0s anyway). This reduces cache utilization. > > > > Good proposal. But I think it could only work with Tx, where the driver > > knows whether the headers are all 0s while filling the desc. But for > > Rx, whereas the driver has to pre-fill the desc, it doesn't know. Thus > > it still requires filling an header desc for storing it. > > I don't see why - I don't think drivers pre-fill buffers in header for RX > right now. Why would they start? Again, my bad, I meant "prepare" but not "pre-fill". Let me try to explain it again. I'm thinking: - For Tx, when the header is all 0s, the header could be discarded. We could use one desc for transfering a packet (with a flag NO_HEADER or HEADER_ALL_ZERO bit set) - For Rx, the header is filled in the device (or vhost) side. And the driver has to prepare the header desc for each pkt, because the Rx driver has no idea whether it will be all 0s. That means, the header could not be discarded. If such a global feature is negotiated, we could also discard the header desc as well. --yliu > > Maybe we could introduce a global feature? When that's negotiated, no > > header desc need filled and processed? I'm thinking this could also > > help the vector implementation I mentioned in another email. > > It's possible of course - it's a subset of what I said. > Though it makes it less useful in the general case. > > > > Note: this feature can be supported in virtio 1.0 as well, > > > as we have unused bits in both descriptor and used ring there. > > > > Agreed. > > > > --yliu > > --------------------------------------------------------------------- > To unsubscribe, e-mail: virtio-dev-unsubscribe@xxxxxxxxxxxxxxxxxxxx > For additional commands, e-mail: virtio-dev-help@xxxxxxxxxxxxxxxxxxxx _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
