Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1565257 > unrolled thread

[PATCH v2] virtio_net: fix PAGE_SIZE > 64k

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] virtio_net: fix PAGE_SIZE > 64k "Michael S. Tsirkin" <mst@redhat.com> - 2017-01-23 20:40 +0100
    Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k David Miller <davem@davemloft.net> - 2017-01-24 20:50 +0100
      Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k "Michael S. Tsirkin" <mst@redhat.com> - 2017-01-24 21:00 +0100
        Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k David Miller <davem@davemloft.net> - 2017-01-24 21:20 +0100
          Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k "Michael S. Tsirkin" <mst@redhat.com> - 2017-01-24 21:50 +0100
            Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k David Miller <davem@davemloft.net> - 2017-01-24 22:00 +0100
              Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k "Michael S. Tsirkin" <mst@redhat.com> - 2017-01-24 22:10 +0100
                Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k David Miller <davem@davemloft.net> - 2017-01-24 22:20 +0100
                  Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k "Michael S. Tsirkin" <mst@redhat.com> - 2017-01-24 23:00 +0100
                    Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k John Fastabend <john.fastabend@gmail.com> - 2017-01-25 04:50 +0100

#1565257 — [PATCH v2] virtio_net: fix PAGE_SIZE > 64k

I don't have any guests with PAGE_SIZE > 64k but the
code seems to be clearly broken in that case
as PAGE_SIZE / MERGEABLE_BUFFER_ALIGN will need
more than 8 bit and so the code in mergeable_ctx_to_buf_address
does not give us the actual true size.

Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

changes from v1:
	fix build warnings

 drivers/net/virtio_net.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4a10500..4dc373b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -48,8 +48,16 @@ module_param(gso, bool, 0444);
  */
 DECLARE_EWMA(pkt_len, 1, 64)
 
+/* With mergeable buffers we align buffer address and use the low bits to
+ * encode its true size. Buffer size is up to 1 page so we need to align to
+ * square root of page size to ensure we reserve enough bits to encode the true
+ * size.
+ */
+#define MERGEABLE_BUFFER_MIN_ALIGN_SHIFT ((PAGE_SHIFT + 1) / 2)
+
 /* Minimum alignment for mergeable packet buffers. */
-#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, 256)
+#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, \
+				   1 << MERGEABLE_BUFFER_MIN_ALIGN_SHIFT)
 
 #define VIRTNET_DRIVER_VERSION "1.0.0"
 
-- 
MST

[toc] | [next] | [standalone]

#1566085

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 21:37:52 +0200

> I don't have any guests with PAGE_SIZE > 64k but the
> code seems to be clearly broken in that case
> as PAGE_SIZE / MERGEABLE_BUFFER_ALIGN will need
> more than 8 bit and so the code in mergeable_ctx_to_buf_address
> does not give us the actual true size.
> 
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Applied, thanks Michael.

I am really trying to be patient, but we are about to run out of
time for fixing the adjust header XDP stuff.

That should have been resolved in a week or two, but now we're
basically a month or so later.

Please come to some kind of agreement about how to implement this
because we can't let v4.10 go out without this being resolved.

Thank you.

[toc] | [prev] | [next] | [standalone]

#1566096

On Tue, Jan 24, 2017 at 02:42:27PM -0500, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Mon, 23 Jan 2017 21:37:52 +0200
> 
> > I don't have any guests with PAGE_SIZE > 64k but the
> > code seems to be clearly broken in that case
> > as PAGE_SIZE / MERGEABLE_BUFFER_ALIGN will need
> > more than 8 bit and so the code in mergeable_ctx_to_buf_address
> > does not give us the actual true size.
> > 
> > Cc: John Fastabend <john.fastabend@gmail.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Applied, thanks Michael.
> 
> I am really trying to be patient, but we are about to run out of
> time for fixing the adjust header XDP stuff.
> 
> That should have been resolved in a week or two, but now we're
> basically a month or so later.
> 
> Please come to some kind of agreement about how to implement this
> because we can't let v4.10 go out without this being resolved.
> 
> Thank you.

I didn't realise. Why can't we? I thought that adjust_header is an
optional feature that userspace can test for, so no rush.

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1566116

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 24 Jan 2017 21:53:13 +0200

> I didn't realise. Why can't we? I thought that adjust_header is an
> optional feature that userspace can test for, so no rush.

No, we want the base set of XDP features to be present in all drivers
supporting XDP.

[toc] | [prev] | [next] | [standalone]

#1566134

On Tue, Jan 24, 2017 at 03:09:59PM -0500, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Tue, 24 Jan 2017 21:53:13 +0200
> 
> > I didn't realise. Why can't we? I thought that adjust_header is an
> > optional feature that userspace can test for, so no rush.
> 
> No, we want the base set of XDP features to be present in all drivers
> supporting XDP.

I see, I didn't realize this. In light of this, is there any
guidance *how much* head room is required to be considered
valid? We already have 12 bytes of headroom.


I'm generally sorry it's taking long, a large part of that is difficulty
figuring out the requirements: when we discussed this on LPC my
take-away was that one of the first users will be fighting DDOS attacks.

In light of this, I assumed that
- supporting just DROP (or DROP+TX) would already be useful
- XDP_PASS shouldn't be too slow as some people will run all
  their traffic with XDP enabled
- people actually want this in virtio because they run in a vm

Since then I heard opinions that seem to imply that
- you must support all features, not just DROP, otherwise it's useless
- XDP_PASS is a slow path fallback as people are not expected to mix XDP
  with regular sockets
- DDOS protection and by extension XDP in virtio is a developer's toy anyway

Thus, expect some slowness while I figure it all out.

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1566139

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 24 Jan 2017 22:45:37 +0200

> On Tue, Jan 24, 2017 at 03:09:59PM -0500, David Miller wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>> Date: Tue, 24 Jan 2017 21:53:13 +0200
>> 
>> > I didn't realise. Why can't we? I thought that adjust_header is an
>> > optional feature that userspace can test for, so no rush.
>> 
>> No, we want the base set of XDP features to be present in all drivers
>> supporting XDP.
> 
> I see, I didn't realize this. In light of this, is there any
> guidance *how much* head room is required to be considered
> valid? We already have 12 bytes of headroom.

The idea is to allow programs to implement arbitrary kinds of
encapsulation, so we need to be able to allow them to push headers for
all kinds of software tunnels, with allowance for a few depths in some
extreme cases.

In that light, a nice round power of 2 number such as 256 seems quite
reasonable to me.

This seems to be what other XDP implementations in drivers use at the
moment as well.

[toc] | [prev] | [next] | [standalone]

#1566143

On Tue, Jan 24, 2017 at 03:53:31PM -0500, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Tue, 24 Jan 2017 22:45:37 +0200
> 
> > On Tue, Jan 24, 2017 at 03:09:59PM -0500, David Miller wrote:
> >> From: "Michael S. Tsirkin" <mst@redhat.com>
> >> Date: Tue, 24 Jan 2017 21:53:13 +0200
> >> 
> >> > I didn't realise. Why can't we? I thought that adjust_header is an
> >> > optional feature that userspace can test for, so no rush.
> >> 
> >> No, we want the base set of XDP features to be present in all drivers
> >> supporting XDP.
> > 
> > I see, I didn't realize this. In light of this, is there any
> > guidance *how much* head room is required to be considered
> > valid? We already have 12 bytes of headroom.
> 
> The idea is to allow programs to implement arbitrary kinds of
> encapsulation, so we need to be able to allow them to push headers for
> all kinds of software tunnels, with allowance for a few depths in some
> extreme cases.
> 
> In that light, a nice round power of 2 number such as 256 seems quite
> reasonable to me.
> 
> This seems to be what other XDP implementations in drivers use at the
> moment as well.

It bothers me that this becomes a part of userspace ABI.
Apps will see that everyone does 256 and will assume it,
we'll never be able to go back.

This does mean that XDP_PASS will use much more memory
for small packets and by extension need a higher rmem limit.
Would all admins be comfortable with this? Why would they want
to if all their XDP does is DROP?
Why not teach applications to query the headroom?

Or even better, do what we do with skbs and do data copies whenever you
run out of headroom instead of a failure. Anyone using build_skb already
has a ton of tailroom so that will work better.

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1566146

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 24 Jan 2017 23:07:51 +0200

> On Tue, Jan 24, 2017 at 03:53:31PM -0500, David Miller wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>> Date: Tue, 24 Jan 2017 22:45:37 +0200
>> 
>> > On Tue, Jan 24, 2017 at 03:09:59PM -0500, David Miller wrote:
>> >> From: "Michael S. Tsirkin" <mst@redhat.com>
>> >> Date: Tue, 24 Jan 2017 21:53:13 +0200
>> >> 
>> >> > I didn't realise. Why can't we? I thought that adjust_header is an
>> >> > optional feature that userspace can test for, so no rush.
>> >> 
>> >> No, we want the base set of XDP features to be present in all drivers
>> >> supporting XDP.
>> > 
>> > I see, I didn't realize this. In light of this, is there any
>> > guidance *how much* head room is required to be considered
>> > valid? We already have 12 bytes of headroom.
>> 
>> The idea is to allow programs to implement arbitrary kinds of
>> encapsulation, so we need to be able to allow them to push headers for
>> all kinds of software tunnels, with allowance for a few depths in some
>> extreme cases.
>> 
>> In that light, a nice round power of 2 number such as 256 seems quite
>> reasonable to me.
>> 
>> This seems to be what other XDP implementations in drivers use at the
>> moment as well.
> 
> It bothers me that this becomes a part of userspace ABI.
> Apps will see that everyone does 256 and will assume it,
> we'll never be able to go back.
> 
> This does mean that XDP_PASS will use much more memory
> for small packets and by extension need a higher rmem limit.
> Would all admins be comfortable with this? Why would they want
> to if all their XDP does is DROP?
> Why not teach applications to query the headroom?

This works in the regimen that XDP packets always live in exactly one
page.  That will be needed to mmap the RX ring into userspace, and it
helps make adjust_header trivial as well.

MTU 1500, PAGESIZE >= 4096, so a headroom of 256 is no problem, and
we still have enough tailroom for skb_shared_info should we wrap
the buffer into a real SKB and push it into the stack.

If you are trying to do buffering differently for virtio_net, well...
that's a self inflicted wound as far as I can tell.

[toc] | [prev] | [next] | [standalone]

#1566172

On Tue, Jan 24, 2017 at 04:10:46PM -0500, David Miller wrote:
> This works in the regimen that XDP packets always live in exactly one
> page.  That will be needed to mmap the RX ring into userspace, and it
> helps make adjust_header trivial as well.

I think the point was to avoid resets across xdp attach/detach.  If we
are doing resets now, we could do whatever buffering we want. We could
also just disable mergeable buffers for that matter.

> MTU 1500, PAGESIZE >= 4096, so a headroom of 256 is no problem, and
> we still have enough tailroom for skb_shared_info should we wrap
> the buffer into a real SKB and push it into the stack.
> 
> If you are trying to do buffering differently for virtio_net, well...
> that's a self inflicted wound as far as I can tell.

Right but I was wondering about the fact that this makes XDP_PASS
much slower than processing skbs without XDP, as truesize is huge
so we'll quickly run out of rmem space.

When XDP is used to fight DOS attacks, why isn't this a concern?

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1566310

On 17-01-24 01:56 PM, Michael S. Tsirkin wrote:
> On Tue, Jan 24, 2017 at 04:10:46PM -0500, David Miller wrote:
>> This works in the regimen that XDP packets always live in exactly one
>> page.  That will be needed to mmap the RX ring into userspace, and it
>> helps make adjust_header trivial as well.

I still don't see why this is a hard requirement for mmap let me post
some patches later tonight to show how we do this with af_packet.

> 
> I think the point was to avoid resets across xdp attach/detach.  If we
> are doing resets now, we could do whatever buffering we want. We could
> also just disable mergeable buffers for that matter.
> 
>> MTU 1500, PAGESIZE >= 4096, so a headroom of 256 is no problem, and
>> we still have enough tailroom for skb_shared_info should we wrap
>> the buffer into a real SKB and push it into the stack.
>>
>> If you are trying to do buffering differently for virtio_net, well...
>> that's a self inflicted wound as far as I can tell.
> 
> Right but I was wondering about the fact that this makes XDP_PASS
> much slower than processing skbs without XDP, as truesize is huge
> so we'll quickly run out of rmem space.
> 
> When XDP is used to fight DOS attacks, why isn't this a concern?
> 

It is a concern on my side. I want XDP and Linux stack to work
reasonably well together.

.John

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web