PACKET_TX_RING: packet size is too long

PACKET_TX_RING: packet size is too long

[Gabor Gombas]

Hi,

I have added PACKET_TX_RING support to ggaoed
(http://code.google.com/p/ggaoed) and it have worked nice when I have
tested it with a virtual ethernet pair (veth). However when testing it
on real hardware, I get:

Oct  8 17:30:52 storage2 kernel: [ 1083.714204] packet size is too long (8740 > 8472)

Kernel is 2.6.31 from Debian experimental. The socket is SOCK_RAW, the
MTU is 9000, I'm using TPACKET_V2 format, and the ring have been created
with the following parameters:

block_nr = 256, block_size = 65536, frame_nr = 1792, frame_size = 9056

Documentation/networking/packet_mmap.txt says that a frame for
PACKET_TX_RING consists of a struct tpacket[2]_hdr followed by the data
to send, so IMHO a frame size of 9056 should be enough to send 8740
bytes of data. However, net/packet/af_packet.c has in tpacket_snd():

	size_max = po->tx_ring.frame_size
		- sizeof(struct skb_shared_info)
		- po->tp_hdrlen
		- LL_ALLOCATED_SPACE(dev)
		- sizeof(struct sockaddr_ll);

which is much more than just the struct tpacket[2]_hdr. So which is
right? The code or the documentation? What is the official formula
to compute the required frame length for a SOCK_RAW socket, given the
MTU?

Gabor

-- 
     ---------------------------------------------------------
     MTA SZTAKI Computer and Automation Research Institute
                Hungarian Academy of Sciences
     ---------------------------------------------------------

[PATCH] Re: PACKET_TX_RING: packet size is too long

[Gabor Gombas]

Hi,

Digging list archives I suspect the current value of size_max is the
remnant of the zero-copy mode that was not merged. So I propose the
following patch that IMHO makes the value of size_max consistent with
how the frame is actually handled in tpacket_fill_skb().

If the zero-copy mode is ever to be resurrected, then the user should
explicitely request it, and either the length of the extra padding
should be the same for 32-bit and 64-bit kernels or there must be a way
to query the value at run time.

Gabor

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index f9f7177..745a016 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -985,10 +985,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		goto out_put;
 
 	size_max = po->tx_ring.frame_size
-		- sizeof(struct skb_shared_info)
-		- po->tp_hdrlen
-		- LL_ALLOCATED_SPACE(dev)
-		- sizeof(struct sockaddr_ll);
+		- (po->tp_hdrlen - sizeof(struct sockaddr_ll));
 
 	if (size_max > dev->mtu + reserve)
 		size_max = dev->mtu + reserve;

-- 
     ---------------------------------------------------------
     MTA SZTAKI Computer and Automation Research Institute
                Hungarian Academy of Sciences
     ---------------------------------------------------------

Re: [PATCH] Re: PACKET_TX_RING: packet size is too long

[David Miller]

From: Gabor Gombas <gombasg@sztaki.hu>
Date: Sat, 10 Oct 2009 00:05:46 +0200

> Hi,
> 
> Digging list archives I suspect the current value of size_max is the
> remnant of the zero-copy mode that was not merged. So I propose the
> following patch that IMHO makes the value of size_max consistent with
> how the frame is actually handled in tpacket_fill_skb().
> 
> If the zero-copy mode is ever to be resurrected, then the user should
> explicitely request it, and either the length of the extra padding
> should be the same for 32-bit and 64-bit kernels or there must be a way
> to query the value at run time.

Johann, please take a look at this.

Gabor, please resubmit your patch with a proper Signed-off-by:
tag so I can apply it if it is correct.

Thanks.

Re: [PATCH] Re: PACKET_TX_RING: packet size is too long

[Gabor Gombas]

Currently PACKET_TX_RING forces certain amount of every frame to remain
unused. This probably originates from an early version of the
PACKET_TX_RING patch that in fact used the extra space when the (since
removed) CONFIG_PACKET_MMAP_ZERO_COPY option was enabled. The current
code does not make any use of this extra space.

This patch removes the extra space reservation and lets userspace make
use of the full frame size.

Signed-off-by: Gabor Gombas <gombasg@sztaki.hu>
---

On Tue, Oct 13, 2009 at 12:54:38AM -0700, David Miller wrote:

> Gabor, please resubmit your patch with a proper Signed-off-by:
> tag so I can apply it if it is correct.

Sure. I have pumped out more than 150 GiB of data using this patch
without apparent ill effects so it is also tested now.

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index f9f7177..745a016 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -985,10 +985,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		goto out_put;
 
 	size_max = po->tx_ring.frame_size
-		- sizeof(struct skb_shared_info)
-		- po->tp_hdrlen
-		- LL_ALLOCATED_SPACE(dev)
-		- sizeof(struct sockaddr_ll);
+		- (po->tp_hdrlen - sizeof(struct sockaddr_ll));
 
 	if (size_max > dev->mtu + reserve)
 		size_max = dev->mtu + reserve;
-- 
1.6.5

-- 
     ---------------------------------------------------------
     MTA SZTAKI Computer and Automation Research Institute
                Hungarian Academy of Sciences
     ---------------------------------------------------------

Re: [PATCH] Re: PACKET_TX_RING: packet size is too long

[David Miller]

From: Gabor Gombas <gombasg@sztaki.hu>
Date: Thu, 15 Oct 2009 22:10:29 +0200

> Currently PACKET_TX_RING forces certain amount of every frame to remain
> unused. This probably originates from an early version of the
> PACKET_TX_RING patch that in fact used the extra space when the (since
> removed) CONFIG_PACKET_MMAP_ZERO_COPY option was enabled. The current
> code does not make any use of this extra space.
> 
> This patch removes the extra space reservation and lets userspace make
> use of the full frame size.
> 
> Signed-off-by: Gabor Gombas <gombasg@sztaki.hu>

Applied, thanks!

Re: [PATCH] Re: PACKET_TX_RING: packet size is too long

[Gabor Gombas]

On Thu, Oct 29, 2009 at 03:19:17AM -0700, David Miller wrote:

> From: Gabor Gombas <gombasg@sztaki.hu>
> Date: Thu, 15 Oct 2009 22:10:29 +0200
> 
> > Currently PACKET_TX_RING forces certain amount of every frame to remain
> > unused. This probably originates from an early version of the
> > PACKET_TX_RING patch that in fact used the extra space when the (since
> > removed) CONFIG_PACKET_MMAP_ZERO_COPY option was enabled. The current
> > code does not make any use of this extra space.
> > 
> > This patch removes the extra space reservation and lets userspace make
> > use of the full frame size.
> > 
> > Signed-off-by: Gabor Gombas <gombasg@sztaki.hu>
> 
> Applied, thanks!

Thanks! Does it have a chance to be included in 2.6.32, or it is too
late in the rc series?

Gabor

-- 
     ---------------------------------------------------------
     MTA SZTAKI Computer and Automation Research Institute
                Hungarian Academy of Sciences,
     ---------------------------------------------------------

Re: [PATCH] Re: PACKET_TX_RING: packet size is too long

[David Miller]

From: Gabor Gombas <gombasg@sztaki.hu>
Date: Thu, 29 Oct 2009 11:29:24 +0100

> On Thu, Oct 29, 2009 at 03:19:17AM -0700, David Miller wrote:
> 
>> From: Gabor Gombas <gombasg@sztaki.hu>
>> Date: Thu, 15 Oct 2009 22:10:29 +0200
>> 
>> > Currently PACKET_TX_RING forces certain amount of every frame to remain
>> > unused. This probably originates from an early version of the
>> > PACKET_TX_RING patch that in fact used the extra space when the (since
>> > removed) CONFIG_PACKET_MMAP_ZERO_COPY option was enabled. The current
>> > code does not make any use of this extra space.
>> > 
>> > This patch removes the extra space reservation and lets userspace make
>> > use of the full frame size.
>> > 
>> > Signed-off-by: Gabor Gombas <gombasg@sztaki.hu>
>> 
>> Applied, thanks!
> 
> Thanks! Does it have a chance to be included in 2.6.32, or it is too
> late in the rc series?

It will be merged to Linus for 2.6.23