[PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

[PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

[Eli Cohen]

>From 59c28d377ddfb7c13e2e1ecbf184211704d0e61d Mon Sep 17 00:00:00 2001
From: Eli Cohen <eli@mellanox.co.il>
Date: Tue, 24 Jun 2008 17:17:14 +0300
Subject: [PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

When an SKB cannot be chained to a session, the current code attempts
to "restore" its ip_summed field from lro_mgr->ip_summed. However,
lro_mgr->ip_summed does not hold the original value; in fact, we'd
better not touch skb->ip_summed since it is not modified by the code
in the path leading to a failure to chain it.

Issue raised by Or Gerlitz <ogerlitz@voltaire.com>

Signed-off-by: Eli Cohen <eli@mellanox.co.il>
---
 net/ipv4/inet_lro.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/inet_lro.c b/net/ipv4/inet_lro.c
index 4a4d49f..cfd034a 100644
--- a/net/ipv4/inet_lro.c
+++ b/net/ipv4/inet_lro.c
@@ -383,8 +383,7 @@ static int __lro_proc_skb(struct net_lro_mgr *lro_mgr, struct sk_buff *skb,
 out2: /* send aggregated SKBs to stack */
 	lro_flush(lro_mgr, lro_desc);
 
-out:  /* Original SKB has to be posted to stack */
-	skb->ip_summed = lro_mgr->ip_summed;
+out:
 	return 1;
 }
 
-- 
1.5.6

Re: [ofa-general] [PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

[Or Gerlitz]

Eli Cohen wrote:
> When an SKB cannot be chained to a session, the current code attempts to "restore" its ip_summed field from lro_mgr->ip_summed. However, lro_mgr->ip_summed does not hold the original value; in fact, we'd better not touch skb->ip_summed since it is not modified by the code in the path leading to a failure to chain it.
>
> --- a/net/ipv4/inet_lro.c
> +++ b/net/ipv4/inet_lro.c
> @@ -383,8 +383,7 @@ static int __lro_proc_skb(struct net_lro_mgr *lro_mgr, struct sk_buff *skb,
>  out2: /* send aggregated SKBs to stack */
>  	lro_flush(lro_mgr, lro_desc);
>  
> -out:  /* Original SKB has to be posted to stack */
> -	skb->ip_summed = lro_mgr->ip_summed;
> +out:
>  	return 1;
>  }
Jan-Bernd,

I understand from your response that lro_mgr->ip_summed is not needed, 
so I guess it should removed from all other places that (eg its 
definition and usage in inet_lro.[ch] and under drivers/net.

Second, if lro_mgr->aggr_ip_summed is indeed needed, I tend to think it 
need to be derived per received packet from skb->ip_summed, since the 
kernel allows for drivers ti have different checksum offload 
capabilities which for some drivers might be impossible to be encoded in 
one global value (lro_mgr->aggr_ip_summed), what's your thinking here?

Third, consider a case where the receiver gets some very small data 
chunks (eg file/block target that has to serve lots of IOPS for some 
clients but also large IOs for other clients), that is some senders set 
TCP_NODELAY, etc. Now, looking in the code _lro_proc_skb() (below) and 
doing reading some reads at the archives, my understanding is that its 
very possible that a large set of small packets would be gathered and 
sent up to the stack only by the consumer calling lro_flush_all in the 
end of its NAPI poll loop. Am I correct?

Or

> static int __lro_proc_skb(struct net_lro_mgr *lro_mgr, struct sk_buff *skb,
> 			  struct vlan_group *vgrp, u16 vlan_tag, void *priv)
> {
> 	struct net_lro_desc *lro_desc;
> 	struct iphdr *iph;
> 	struct tcphdr *tcph;
> 	u64 flags;
> 	int vlan_hdr_len = 0;
>
> 	if (!lro_mgr->get_skb_header
> 	    || lro_mgr->get_skb_header(skb, (void *)&iph, (void *)&tcph,
> 				       &flags, priv))
> 		goto out;
>
> 	if (!(flags & LRO_IPV4) || !(flags & LRO_TCP))
> 		goto out;
>
> 	lro_desc = lro_get_desc(lro_mgr, lro_mgr->lro_arr, iph, tcph);
> 	if (!lro_desc)
> 		goto out;
>
> 	if ((skb->protocol == htons(ETH_P_8021Q))
> 	    && !(lro_mgr->features & LRO_F_EXTRACT_VLAN_ID))
> 		vlan_hdr_len = VLAN_HLEN;
>
> 	if (!lro_desc->active) { /* start new lro session */
> 		if (lro_tcp_ip_check(iph, tcph, skb->len - vlan_hdr_len, NULL))
> 			goto out;
>
> 		skb->ip_summed = lro_mgr->ip_summed_aggr;
> 		lro_init_desc(lro_desc, skb, iph, tcph, vlan_tag, vgrp);
> 		LRO_INC_STATS(lro_mgr, aggregated);
> 		return 0;
> 	}
>
> 	if (lro_desc->tcp_next_seq != ntohl(tcph->seq))
> 		goto out2;
>
> 	if (lro_tcp_ip_check(iph, tcph, skb->len, lro_desc))
> 		goto out2;
>
> 	lro_add_packet(lro_desc, skb, iph, tcph);
> 	LRO_INC_STATS(lro_mgr, aggregated);
>
> 	if ((lro_desc->pkt_aggr_cnt >= lro_mgr->max_aggr) ||
> 	    lro_desc->parent->len > (0xFFFF - lro_mgr->dev->mtu))
> 		lro_flush(lro_mgr, lro_desc);
>
> 	return 0;
>
> out2: /* send aggregated SKBs to stack */
> 	lro_flush(lro_mgr, lro_desc);
>
> out:  /* Original SKB has to be posted to stack */
> 	skb->ip_summed = lro_mgr->ip_summed;
> 	return 1;
> }

 

Re: [ofa-general] [PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

[Jan-Bernd Themann]

[-- Attachment #1.1: Type: text/plain, Size: 2257 bytes --]

Hi

Or Gerlitz <ogerlitz@voltaire.com> wrote on 25.06.2008 11:26:12:

> Jan-Bernd,
> 
> I understand from your response that lro_mgr->ip_summed is not needed, 
> so I guess it should removed from all other places that (eg its 
> definition and usage in inet_lro.[ch] and under drivers/net.
> 

no, what I meant is that it is only not needed at that particular place as 

the packet is not handled by LRO. Without this line the driver can set an 
individual 
value for each SKB that is not aggregated if wished. For example when the 
packet is not a valid IP packet.
However, removing all ip_summed fields impacts the fragment lro mode. 
There we have to set
some value for not aggregated packets. The SKBs are generated within the 
LRO engine.
If desired (and if there is HW that wants to use that) 
we can pass that value for each provided fragment. This would add one 
additional paramter to the already 8 parameters
of __lro_proc_segment. That is of course possible.


> Second, if lro_mgr->aggr_ip_summed is indeed needed, I tend to think it 
> need to be derived per received packet from skb->ip_summed, since the 
> kernel allows for drivers ti have different checksum offload 
> capabilities which for some drivers might be impossible to be encoded in 

> one global value (lro_mgr->aggr_ip_summed), what's your thinking here?

I think that for valid TCP/IP packets this value should always be the same
as the hardware either support the set ip_summed_aggr value for TCP/IPv4 
packets, or not.
Maybe that assumption is not right, but so far I haven't seen any hardware 
that behaves
in a different way.

> 
> Third, consider a case where the receiver gets some very small data 
> chunks (eg file/block target that has to serve lots of IOPS for some 
> clients but also large IOs for other clients), that is some senders set 
> TCP_NODELAY, etc. Now, looking in the code _lro_proc_skb() (below) and 
> doing reading some reads at the archives, my understanding is that its 
> very possible that a large set of small packets would be gathered and 
> sent up to the stack only by the consumer calling lro_flush_all in the 
> end of its NAPI poll loop. Am I correct?

yes, that is possible. An increased delay is the prise of LRO :-)

Regards,
Jan-Bernd

[-- Attachment #1.2: Type: text/html, Size: 3066 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ofa-general] [PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

[Or Gerlitz]

Jan-Bernd Themann wrote:
> no, what I meant is that it is only not needed at that particular 
> place as  the packet is not handled by LRO. Without this line the 
> driver can set an individual  value for each SKB that is not 
> aggregated if wished. For example when the packet is not a valid IP 
> packet.  However, removing all ip_summed fields impacts the fragment 
> lro mode. There we have to set some value for not aggregated packets. 
> The SKBs are generated within the LRO engine. If desired (and if there 
> is HW that wants to use that) we can pass that value for each provided 
> fragment. This would add one additional paramter to the already 8 
> parameters of __lro_proc_segment. That is of course possible.
OK, understood, both points. 

Eli, lets add to this patch a comment in inet_lro.h saying that the 
value of lro_mgr->ip_summed is ignored by the core lro code for drivers 
that use the non fragmented mode. Also for the ipoib patch, lets not set 
this value.

> I think that for valid TCP/IP packets this value should always be the 
> same as the hardware either support the set ip_summed_aggr value for 
> TCP/IPv4 packets, or not. Maybe that assumption is not right, but so 
> far I haven't seen any hardware that behaves in a different way.
Yes, for TCP/IPv4 you seem to be right and here the problem was in the 
lro patch to ipoib which set this value blindly regardless of the HW 
capabilities, I asked Vlad to change this in the next version of the 
patch. As for other types of traffic, I was thinking that allowing the 
driver to set it per packet makes a better isolation between the core 
lro code to the driver, but this is not major issue.
> yes, that is possible. An increased delay is the prise of LRO :-)
>
Is there some pointer you might be able to provide on LRO benchmark for 
small packets and/or mixed small/large packet streams?

Or.

Re: [ofa-general] [PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

[Jan-Bernd Themann]

[-- Attachment #1.1: Type: text/plain, Size: 2025 bytes --]

Or Gerlitz <ogerlitz@voltaire.com> wrote on 25.06.2008 13:47:25:

> Jan-Bernd Themann wrote:
> > no, what I meant is that it is only not needed at that particular 
> > place as  the packet is not handled by LRO. Without this line the 
> > driver can set an individual  value for each SKB that is not 
> > aggregated if wished. For example when the packet is not a valid IP 
> > packet.  However, removing all ip_summed fields impacts the fragment 
> > lro mode. There we have to set some value for not aggregated packets. 
> > The SKBs are generated within the LRO engine. If desired (and if there 

> > is HW that wants to use that) we can pass that value for each provided 

> > fragment. This would add one additional paramter to the already 8 
> > parameters of __lro_proc_segment. That is of course possible.
> OK, understood, both points. 
> 
> Eli, lets add to this patch a comment in inet_lro.h saying that the 
> value of lro_mgr->ip_summed is ignored by the core lro code for drivers 
> that use the non fragmented mode. Also for the ipoib patch, lets not set 

> this value.
> 
> > I think that for valid TCP/IP packets this value should always be the 
> > same as the hardware either support the set ip_summed_aggr value for 
> > TCP/IPv4 packets, or not. Maybe that assumption is not right, but so 
> > far I haven't seen any hardware that behaves in a different way.
> Yes, for TCP/IPv4 you seem to be right and here the problem was in the 
> lro patch to ipoib which set this value blindly regardless of the HW 
> capabilities, I asked Vlad to change this in the next version of the 
> patch. As for other types of traffic, I was thinking that allowing the 
> driver to set it per packet makes a better isolation between the core 
> lro code to the driver, but this is not major issue.
> > yes, that is possible. An increased delay is the prise of LRO :-)
> >
> Is there some pointer you might be able to provide on LRO benchmark for 
> small packets and/or mixed small/large packet streams?



> 
> Or.
> 
> 

[-- Attachment #1.2: Type: text/html, Size: 2429 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ofa-general] [PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

[Jan-Bernd Themann]

[-- Attachment #1.1: Type: text/plain, Size: 2239 bytes --]

Hi Or,

sorry, had a problem with the last mail...

Or Gerlitz <ogerlitz@voltaire.com> wrote on 25.06.2008 13:47:25:

> Jan-Bernd Themann wrote:
> > no, what I meant is that it is only not needed at that particular 
> > place as  the packet is not handled by LRO. Without this line the 
> > driver can set an individual  value for each SKB that is not 
> > aggregated if wished. For example when the packet is not a valid IP 
> > packet.  However, removing all ip_summed fields impacts the fragment 
> > lro mode. There we have to set some value for not aggregated packets. 
> > The SKBs are generated within the LRO engine. If desired (and if there 

> > is HW that wants to use that) we can pass that value for each provided 

> > fragment. This would add one additional paramter to the already 8 
> > parameters of __lro_proc_segment. That is of course possible.
> OK, understood, both points. 
> 
> Eli, lets add to this patch a comment in inet_lro.h saying that the 
> value of lro_mgr->ip_summed is ignored by the core lro code for drivers 
> that use the non fragmented mode. Also for the ipoib patch, lets not set 

> this value.
> 
> > I think that for valid TCP/IP packets this value should always be the 
> > same as the hardware either support the set ip_summed_aggr value for 
> > TCP/IPv4 packets, or not. Maybe that assumption is not right, but so 
> > far I haven't seen any hardware that behaves in a different way.
> Yes, for TCP/IPv4 you seem to be right and here the problem was in the 
> lro patch to ipoib which set this value blindly regardless of the HW 
> capabilities, I asked Vlad to change this in the next version of the 
> patch. As for other types of traffic, I was thinking that allowing the 
> driver to set it per packet makes a better isolation between the core 
> lro code to the driver, but this is not major issue.
> > yes, that is possible. An increased delay is the prise of LRO :-)
> >
> Is there some pointer you might be able to provide on LRO benchmark for 
> small packets and/or mixed small/large packet streams?

I don't have any benchmarks handy, but Andrew Gallatin from myrinet posted 
his results on
the mailing list some month ago. I'll let you know if I find a link.

Regards,
Jan-Bernd

[-- Attachment #1.2: Type: text/html, Size: 2783 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ofa-general] [PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

[Eli Cohen]

On Wed, 2008-06-25 at 14:47 +0300, Or Gerlitz wrote:
> Jan-Bernd Themann wrote:
> > no, what I meant is that it is only not needed at that particular 
> > place as  the packet is not handled by LRO. Without this line the 
> > driver can set an individual  value for each SKB that is not 
> > aggregated if wished. For example when the packet is not a valid IP 
> > packet.  However, removing all ip_summed fields impacts the fragment 
> > lro mode. There we have to set some value for not aggregated packets. 
> > The SKBs are generated within the LRO engine. If desired (and if there 
> > is HW that wants to use that) we can pass that value for each provided 
> > fragment. This would add one additional paramter to the already 8 
> > parameters of __lro_proc_segment. That is of course possible.
> OK, understood, both points. 
> 
> Eli, lets add to this patch a comment in inet_lro.h saying that the 
> value of lro_mgr->ip_summed is ignored by the core lro code for drivers 
> that use the non fragmented mode.
We already have this comment:

struct net_lro_mgr {
	...
	u32 ip_summed;      /* Set in non generated SKBs in page mode */

I think we should use have it something like that:
	...
	/*
	 * Set for generated SKBs in that are not added to
	 * the frag list in fragmented mode
	 */
	u32 ip_summed;

What do you think?


>  Also for the ipoib patch, lets not set 
> this value.
Agree.

Re: [ofa-general] [PATCH] net/inet_lro: remove setting skb->ip_summed when not LRO-able

[Or Gerlitz]

Eli Cohen wrote:
> We already have this comment:
> 	u32 ip_summed;      /* Set in non generated SKBs in page mode */
>
> I think we should use have it something like that:
> 	/*
> 	 * Set for generated SKBs in that are not added to
> 	 * the frag list in fragmented mode
> 	 */
> 	u32 ip_summed;
>
> What do you think?
>   
yes, this could be a little more clear.


Or