On Thu, 29 Sep 2022 06:06:41 -0400, "Michael S. Tsirkin" <[email protected]>
wrote:
> On Thu, Sep 29, 2022 at 04:24:02PM +0800, Xuan Zhuo wrote:
> > On Thu, 29 Sep 2022 03:04:03 -0400, "Michael S. Tsirkin" <[email protected]>
> > wrote:
> > > On Thu, Sep 29, 2022 at 09:48:33AM +0800, Jason Wang wrote:
> > > > On Wed, Sep 28, 2022 at 9:39 PM Michael S. Tsirkin <[email protected]>
> > > > wrote:
> > > > >
> > > > > On Mon, Sep 26, 2022 at 04:06:17PM +0800, Jason Wang wrote:
> > > > > > > Jason I think the issue with previous proposals is that they
> > > > > > > conflict
> > > > > > > with VIRTIO_F_ANY_LAYOUT. We have repeatedly found that giving the
> > > > > > > driver flexibility in arranging the packet in memory is
> > > > > > > benefitial.
> > > > > >
> > > > > >
> > > > > > Yes, but I didn't found how it can conflict the any_layout. Device
> > > > > > can just
> > > > > > to not split the header when the layout doesn't fit for header
> > > > > > splitting.
> > > > > > (And this seems the case even if we're using buffers).
> > > > >
> > > > > Well spec says:
> > > > >
> > > > > indicates to both the device and the driver that no
> > > > > assumptions were made about framing.
> > > > >
> > > > > if device assumes that descriptor boundaries are where
> > > > > driver wants packet to be stored that is clearly
> > > > > an assumption.
> > > >
> > > > Yes but what I want to say is, the device can choose to not split the
> > > > packet if the framing doesn't fit. Does it still comply with the above
> > > > description?
> > > >
> > > > Thanks
> > >
> > > The point of ANY_LAYOUT is to give drivers maximum flexibility.
> > > For example, if driver wants to split the header at some specific
> > > offset this is already possible without extra functionality.
> > >
> > > Let's keep it that way.
> > >
> > > Now, let's formulate what are some of the problems with the current way.
> > >
> > >
> > >
> > > A- mergeable buffers is even more flexible, since a single packet
> > > is built up of multiple buffers.
> >
> > If I understand correctly, this is our v8.
>
> I think it is, or at least close.
>
> > > And in theory device can
> > > choose arbitrary set of buffers to store a packet.
> > > So you could supply a small buffer for headers followed by a bigger
> > > one for payload, in theory even without any changes.
> >
> > This is very interesting, I did not think of this point.
> > This is helpful to reduce the waste of memory.
>
> Hmm good point. I should add: since we are no longer fully using the
> first buffer the feature has a memory cost and - in case of a cache
> pressure - can degrade performance rather than improve it.
> Thus, allowing flexibility for both devices and drivers at runtime
> rather than fixing things through features thus sounds like a good idea.
>
>
> > > Problem 1: However since this is not how devices currently operate,
> > > a feature bit would be helpful.
> > >
> > > Problem 2: Also, in the past we found it useful to be able to figure
> > > out whether
> > > packet fits in a single buffer without looking at the header.
> > > For this reason, we have this text:
> > >
> > > If a receive packet is spread over multiple buffers, the device
> > > MUST use all buffers but the last (i.e. the first \field{num_buffers} -
> > > 1 buffers) completely up to the full length of each buffer
> > > supplied by the driver.
> > >
> > > if we want to keep this optimization and allow using a separate
> > > buffer for headers, then I think we could rely on the feature bit
> > > from Problem 1 and just make an exception for the first buffer.
> > > Also num_buffers is then always >= 2, maybe state this to avoid
> > > confusion.
> > >
> >
> > Yes, I think this is feasible.
>
>
> Thinking more about this, a question is what to do about packets without
> split header. I can see several options
> A- add some buffers just for the non split case. without in-order they
> can just stay available. with in-order they have to be recycled which
> might be expensive.
> They will also occupy space in the ring. more memory costs.
> Don't much like it for above reasons.
> OTOH then I wanted to work on partial-order anyway. Maybe it's time
> to prioritize that work.
Yes, I also prefer order processing, although we mess up the order, which is
good in some cases.
>
> B- write all of the packet in the payload buffer and just skip header buffer
> (e.g. make it 0 size?)
> payload within packet will be misaligned then. Do we care? maybe not -
> this was supposed to be an exception. In this case:
> Problem B: where should virtio net header
> go then? we can put it in the header buffer still, or we can
> store it linear with the packet. or we can leave both options, up to
> device.
> what is better might depend on
> different factors. any chance of performance testing?
avail ring:
| small buffer | page | small buffer | page | small buffer | page | small
buffer | page |
In this case, I think it is a good idea to set the small buffer to 0 size for
non-split headers.
This is an advantage over desc chain. The desc chain can only put the virtio-net
header in the small buffer.
>
> > >
> > >
> > >
> > >
> > > B- without mergeable, there's no flexibility. In particular, there can
> > > not be uninitialized space between header and data. If we had flexibility
> > > here, this could be
> > > helpful for alignment, security, etc.
> > > Unfortunately, our hands are tied:
> > >
> > >
> > > \field{len} is particularly useful
> > > for drivers using untrusted buffers: if a driver does not know exactly
> > > how much has been written by the device, the driver would have to zero
> > > the buffer in advance to ensure no data leakage occurs.
> > >
> > > For example, a network driver may hand a received buffer directly to
> > > an unprivileged userspace application. If the network device has not
> > > overwritten the bytes which were in that buffer, this could leak the
> > > contents of freed memory from other processes to the application.
> >
> >
> > I don't think this is very troublesome, the device can memset the hole by 0.
>
> Yes it can. But that has a performance cost. How large - depends on the
> hole size and a bunch of other factors.
The actual hole size is not large, general IP + TCP size.
Thanks
>
> > >
> > >
> > > so all buffers have to be initialized completely up to the reported
> > > used length.
> > >
> > > It could be that the guarantee is not relevant in some use-cases.
> > > We would have to specify that this is an exception to the rule,
> > > explain that drivers must be careful about information leaks.
> > > Let's say there's a feature bit that adds uninitialized space
> > > somewhere. How much was added? We can find out by parsing the
> > > packet but once you start doing that just to assemble the skb
> > > you have already lost performance.
> > > So lots of spec work, some security risks, and unclear performance.
> > >
> > >
> > >
> > >
> > > Is above a fair summary?
> > >
> > >
> > >
> > > If yes I would say let's address A, but make sure we ask drivers
> > > to clear the new feature bit if there's no mergeable
> > > (as opposed to e.g. failing probe) so we can add
> > > support for !mergeable down the road.
> > >
> > >
> > >
> > >
> > >
> > > --
> > > MST
> > >
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
