| Message ID | 1361485562.21950.3.camel@lb-tlvb-dmitry |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Feb 21, 2013 at 2:26 PM, Dmitry Kravkov <dmitry@broadcom.com> wrote: > On Tue, 2013-02-19 at 15:03 -0800, pravin wrote: >> On Tue, Feb 19, 2013 at 11:20 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote: >> > >> >> -----Original Message----- >> >> From: pravin [mailto:pravin.shelar@gmail.com] >> >> Sent: Tuesday, February 19, 2013 8:28 PM >> >> To: Dmitry Kravkov >> >> Cc: davem@davemloft.net; netdev@vger.kernel.org >> >> Subject: Re: [PATCH net-next v2 1/2] ip_gre: allow CSUM capable devices to >> >> handle packets >> >> >> >> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@broadcom.com> >> >> wrote: >> >> > If device is not able to handle checksumming it will >> >> > be handled in dev_xmit >> >> > >> >> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com> >> >> > --- >> >> > Changes from v1: fixed email address >> >> > >> >> > net/ipv4/ip_gre.c | 7 ++----- >> >> > 1 files changed, 2 insertions(+), 5 deletions(-) >> >> > >> >> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c >> >> > index a56f118..cdc31ac 100644 >> >> > --- a/net/ipv4/ip_gre.c >> >> > +++ b/net/ipv4/ip_gre.c >> >> > @@ -745,12 +745,9 @@ static struct sk_buff *handle_offloads(struct sk_buff >> >> *skb) >> >> > goto error; >> >> > skb_shinfo(skb)->gso_type |= SKB_GSO_GRE; >> >> > return skb; >> >> > - } else if (skb->ip_summed == CHECKSUM_PARTIAL) { >> >> > - err = skb_checksum_help(skb); >> >> > - if (unlikely(err)) >> >> > - goto error; >> >> > } >> >> > - skb->ip_summed = CHECKSUM_NONE; >> >> > + if (skb->ip_summed != CHECKSUM_PARTIAL) >> >> > + skb->ip_summed = CHECKSUM_NONE; >> >> > >> >> > return skb; >> >> > >> >> > -- >> >> > 1.7.7.2 >> >> > >> >> > >> >> >> >> This patch breaks GRE tunnel with GRE_CSUM. since GRE_CSUM need >> >> complete IP packet to checksum entire GRE payload. >> > >> > Testing for o_flags&GRE_CSUM does not look too hurt here, since it will be used in ipgre_tunnel_xmit() later on >> > This is the only problematic case, right? >> > >> >> It does not work for me. I have GRE device with csum on. Ping works >> fine but Netperf is not working. >> Looking at code, I am not sure how tcp will work if inner packet TCP >> checksum is calculated after GRE_CSUM calculation. > > > Fixes handling when CSUM or SEQ flags are set - via skb_checksum_help() > --- > Tested with each mode (separately) > > net/ipv4/ip_gre.c | 14 +++++++++++--- > 1 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > index 5ef4da7..0de54cd 100644 > --- a/net/ipv4/ip_gre.c > +++ b/net/ipv4/ip_gre.c > @@ -735,7 +735,8 @@ drop: > return 0; > } > > -static struct sk_buff *handle_offloads(struct sk_buff *skb) > +static struct sk_buff *handle_offloads(struct ip_tunnel *tunnel, > + struct sk_buff *skb) > { > int err; > > @@ -745,8 +746,15 @@ static struct sk_buff *handle_offloads(struct > sk_buff *skb) > goto error; > skb_shinfo(skb)->gso_type |= SKB_GSO_GRE; > return skb; > + } else if (tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM) && > + skb->ip_summed == CHECKSUM_PARTIAL) { > + err = skb_checksum_help(skb); > + if (unlikely(err)) > + goto error; > } > - if (skb->ip_summed != CHECKSUM_PARTIAL) > + > + if (skb->ip_summed != CHECKSUM_PARTIAL || > + tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM)) > skb->ip_summed = CHECKSUM_NONE; > We can simplify this by dropping check of o_flags. > return skb; > @@ -776,7 +784,7 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff > *skb, struct net_device *dev > int err; > int pkt_len; > > - skb = handle_offloads(skb); > + skb = handle_offloads(tunnel, skb); > if (IS_ERR(skb)) { > dev->stats.tx_dropped++; > return NETDEV_TX_OK; > -- > 1.7.7.2 > > >> Thanks, >> Pravin. >> > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 21, 2013 at 3:46 PM, pravin <pravin.shelar@gmail.com> wrote: > On Thu, Feb 21, 2013 at 2:26 PM, Dmitry Kravkov <dmitry@broadcom.com> wrote: >> On Tue, 2013-02-19 at 15:03 -0800, pravin wrote: >>> On Tue, Feb 19, 2013 at 11:20 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote: >>> > >>> >> -----Original Message----- >>> >> From: pravin [mailto:pravin.shelar@gmail.com] >>> >> Sent: Tuesday, February 19, 2013 8:28 PM >>> >> To: Dmitry Kravkov >>> >> Cc: davem@davemloft.net; netdev@vger.kernel.org >>> >> Subject: Re: [PATCH net-next v2 1/2] ip_gre: allow CSUM capable devices to >>> >> handle packets >>> >> >>> >> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@broadcom.com> >>> >> wrote: >>> >> > If device is not able to handle checksumming it will >>> >> > be handled in dev_xmit >>> >> > >>> >> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com> >>> >> > --- >>> >> > Changes from v1: fixed email address >>> >> > >>> >> > net/ipv4/ip_gre.c | 7 ++----- >>> >> > 1 files changed, 2 insertions(+), 5 deletions(-) >>> >> > >>> >> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c >>> >> > index a56f118..cdc31ac 100644 >>> >> > --- a/net/ipv4/ip_gre.c >>> >> > +++ b/net/ipv4/ip_gre.c >>> >> > @@ -745,12 +745,9 @@ static struct sk_buff *handle_offloads(struct sk_buff >>> >> *skb) >>> >> > goto error; >>> >> > skb_shinfo(skb)->gso_type |= SKB_GSO_GRE; >>> >> > return skb; >>> >> > - } else if (skb->ip_summed == CHECKSUM_PARTIAL) { >>> >> > - err = skb_checksum_help(skb); >>> >> > - if (unlikely(err)) >>> >> > - goto error; >>> >> > } >>> >> > - skb->ip_summed = CHECKSUM_NONE; >>> >> > + if (skb->ip_summed != CHECKSUM_PARTIAL) >>> >> > + skb->ip_summed = CHECKSUM_NONE; >>> >> > >>> >> > return skb; >>> >> > >>> >> > -- >>> >> > 1.7.7.2 >>> >> > >>> >> > >>> >> >>> >> This patch breaks GRE tunnel with GRE_CSUM. since GRE_CSUM need >>> >> complete IP packet to checksum entire GRE payload. >>> > >>> > Testing for o_flags&GRE_CSUM does not look too hurt here, since it will be used in ipgre_tunnel_xmit() later on >>> > This is the only problematic case, right? >>> > >>> >>> It does not work for me. I have GRE device with csum on. Ping works >>> fine but Netperf is not working. >>> Looking at code, I am not sure how tcp will work if inner packet TCP >>> checksum is calculated after GRE_CSUM calculation. >> >> >> Fixes handling when CSUM or SEQ flags are set - via skb_checksum_help() >> --- >> Tested with each mode (separately) >> >> net/ipv4/ip_gre.c | 14 +++++++++++--- >> 1 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c >> index 5ef4da7..0de54cd 100644 >> --- a/net/ipv4/ip_gre.c >> +++ b/net/ipv4/ip_gre.c >> @@ -735,7 +735,8 @@ drop: >> return 0; >> } >> >> -static struct sk_buff *handle_offloads(struct sk_buff *skb) >> +static struct sk_buff *handle_offloads(struct ip_tunnel *tunnel, >> + struct sk_buff *skb) >> { >> int err; >> >> @@ -745,8 +746,15 @@ static struct sk_buff *handle_offloads(struct >> sk_buff *skb) >> goto error; >> skb_shinfo(skb)->gso_type |= SKB_GSO_GRE; >> return skb; >> + } else if (tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM) && >> + skb->ip_summed == CHECKSUM_PARTIAL) { >> + err = skb_checksum_help(skb); >> + if (unlikely(err)) >> + goto error; >> } one more thing, there is no need to do csum for GRE_SEQ case. >> - if (skb->ip_summed != CHECKSUM_PARTIAL) >> + >> + if (skb->ip_summed != CHECKSUM_PARTIAL || >> + tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM)) >> skb->ip_summed = CHECKSUM_NONE; >> > We can simplify this by dropping check of o_flags. > >> return skb; >> @@ -776,7 +784,7 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff >> *skb, struct net_device *dev >> int err; >> int pkt_len; >> >> - skb = handle_offloads(skb); >> + skb = handle_offloads(tunnel, skb); >> if (IS_ERR(skb)) { >> dev->stats.tx_dropped++; >> return NETDEV_TX_OK; >> -- >> 1.7.7.2 >> >> >>> Thanks, >>> Pravin. >>> >> >> >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-02-21 at 23:19 -0800, pravin wrote: > On Thu, Feb 21, 2013 at 3:46 PM, pravin <pravin.shelar@gmail.com> wrote: > > On Thu, Feb 21, 2013 at 2:26 PM, Dmitry Kravkov <dmitry@broadcom.com> wrote: > >> On Tue, 2013-02-19 at 15:03 -0800, pravin wrote: > >>> On Tue, Feb 19, 2013 at 11:20 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote: > >>> > > >>> >> -----Original Message----- > >>> >> From: pravin [mailto:pravin.shelar@gmail.com] > >>> >> Sent: Tuesday, February 19, 2013 8:28 PM > >>> >> To: Dmitry Kravkov > >>> >> Cc: davem@davemloft.net; netdev@vger.kernel.org > >>> >> Subject: Re: [PATCH net-next v2 1/2] ip_gre: allow CSUM capable devices to > >>> >> handle packets > >>> >> > >>> >> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@broadcom.com> > >>> >> wrote: > >>> >> > If device is not able to handle checksumming it will > >>> >> > be handled in dev_xmit > >>> >> > > >>> >> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com> > >>> >> > --- > >>> >> > Changes from v1: fixed email address > >>> >> > > >>> >> > net/ipv4/ip_gre.c | 7 ++----- > >>> >> > 1 files changed, 2 insertions(+), 5 deletions(-) > >>> >> > > >>> >> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > >>> >> > index a56f118..cdc31ac 100644 > >>> >> > --- a/net/ipv4/ip_gre.c > >>> >> > +++ b/net/ipv4/ip_gre.c > >>> >> > @@ -745,12 +745,9 @@ static struct sk_buff *handle_offloads(struct sk_buff > >>> >> *skb) > >>> >> > goto error; > >>> >> > skb_shinfo(skb)->gso_type |= SKB_GSO_GRE; > >>> >> > return skb; > >>> >> > - } else if (skb->ip_summed == CHECKSUM_PARTIAL) { > >>> >> > - err = skb_checksum_help(skb); > >>> >> > - if (unlikely(err)) > >>> >> > - goto error; > >>> >> > } > >>> >> > - skb->ip_summed = CHECKSUM_NONE; > >>> >> > + if (skb->ip_summed != CHECKSUM_PARTIAL) > >>> >> > + skb->ip_summed = CHECKSUM_NONE; > >>> >> > > >>> >> > return skb; > >>> >> > > >>> >> > -- > >>> >> > 1.7.7.2 > >>> >> > > >>> >> > > >>> >> > >>> >> This patch breaks GRE tunnel with GRE_CSUM. since GRE_CSUM need > >>> >> complete IP packet to checksum entire GRE payload. > >>> > > >>> > Testing for o_flags&GRE_CSUM does not look too hurt here, since it will be used in ipgre_tunnel_xmit() later on > >>> > This is the only problematic case, right? > >>> > > >>> > >>> It does not work for me. I have GRE device with csum on. Ping works > >>> fine but Netperf is not working. > >>> Looking at code, I am not sure how tcp will work if inner packet TCP > >>> checksum is calculated after GRE_CSUM calculation. > >> > >> > >> Fixes handling when CSUM or SEQ flags are set - via skb_checksum_help() > >> --- > >> Tested with each mode (separately) > >> > >> net/ipv4/ip_gre.c | 14 +++++++++++--- > >> 1 files changed, 11 insertions(+), 3 deletions(-) > >> > >> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > >> index 5ef4da7..0de54cd 100644 > >> --- a/net/ipv4/ip_gre.c > >> +++ b/net/ipv4/ip_gre.c > >> @@ -735,7 +735,8 @@ drop: > >> return 0; > >> } > >> > >> -static struct sk_buff *handle_offloads(struct sk_buff *skb) > >> +static struct sk_buff *handle_offloads(struct ip_tunnel *tunnel, > >> + struct sk_buff *skb) > >> { > >> int err; > >> > >> @@ -745,8 +746,15 @@ static struct sk_buff *handle_offloads(struct > >> sk_buff *skb) > >> goto error; > >> skb_shinfo(skb)->gso_type |= SKB_GSO_GRE; > >> return skb; > >> + } else if (tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM) && > >> + skb->ip_summed == CHECKSUM_PARTIAL) { > >> + err = skb_checksum_help(skb); > >> + if (unlikely(err)) > >> + goto error; > >> } > > one more thing, there is no need to do csum for GRE_SEQ case. If csum is not performed TCP csum become incorrect for offload capable and incapable devices. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 24, 2013 at 8:56 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote: > On Thu, 2013-02-21 at 23:19 -0800, pravin wrote: >> On Thu, Feb 21, 2013 at 3:46 PM, pravin <pravin.shelar@gmail.com> wrote: >> > On Thu, Feb 21, 2013 at 2:26 PM, Dmitry Kravkov <dmitry@broadcom.com> wrote: >> >> On Tue, 2013-02-19 at 15:03 -0800, pravin wrote: >> >>> On Tue, Feb 19, 2013 at 11:20 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote: >> >>> > >> >>> >> -----Original Message----- >> >>> >> From: pravin [mailto:pravin.shelar@gmail.com] >> >>> >> Sent: Tuesday, February 19, 2013 8:28 PM >> >>> >> To: Dmitry Kravkov >> >>> >> Cc: davem@davemloft.net; netdev@vger.kernel.org >> >>> >> Subject: Re: [PATCH net-next v2 1/2] ip_gre: allow CSUM capable devices to >> >>> >> handle packets >> >>> >> >> >>> >> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@broadcom.com> >> >>> >> wrote: >> >>> >> > If device is not able to handle checksumming it will >> >>> >> > be handled in dev_xmit >> >>> >> > >> >>> >> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com> >> >>> >> > --- >> >>> >> > Changes from v1: fixed email address >> >>> >> > >> >>> >> > net/ipv4/ip_gre.c | 7 ++----- >> >>> >> > 1 files changed, 2 insertions(+), 5 deletions(-) >> >>> >> > >> >>> >> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c >> >>> >> > index a56f118..cdc31ac 100644 >> >>> >> > --- a/net/ipv4/ip_gre.c >> >>> >> > +++ b/net/ipv4/ip_gre.c >> >>> >> > @@ -745,12 +745,9 @@ static struct sk_buff *handle_offloads(struct sk_buff >> >>> >> *skb) >> >>> >> > goto error; >> >>> >> > skb_shinfo(skb)->gso_type |= SKB_GSO_GRE; >> >>> >> > return skb; >> >>> >> > - } else if (skb->ip_summed == CHECKSUM_PARTIAL) { >> >>> >> > - err = skb_checksum_help(skb); >> >>> >> > - if (unlikely(err)) >> >>> >> > - goto error; >> >>> >> > } >> >>> >> > - skb->ip_summed = CHECKSUM_NONE; >> >>> >> > + if (skb->ip_summed != CHECKSUM_PARTIAL) >> >>> >> > + skb->ip_summed = CHECKSUM_NONE; >> >>> >> > >> >>> >> > return skb; >> >>> >> > >> >>> >> > -- >> >>> >> > 1.7.7.2 >> >>> >> > >> >>> >> > >> >>> >> >> >>> >> This patch breaks GRE tunnel with GRE_CSUM. since GRE_CSUM need >> >>> >> complete IP packet to checksum entire GRE payload. >> >>> > >> >>> > Testing for o_flags&GRE_CSUM does not look too hurt here, since it will be used in ipgre_tunnel_xmit() later on >> >>> > This is the only problematic case, right? >> >>> > >> >>> >> >>> It does not work for me. I have GRE device with csum on. Ping works >> >>> fine but Netperf is not working. >> >>> Looking at code, I am not sure how tcp will work if inner packet TCP >> >>> checksum is calculated after GRE_CSUM calculation. >> >> >> >> >> >> Fixes handling when CSUM or SEQ flags are set - via skb_checksum_help() >> >> --- >> >> Tested with each mode (separately) >> >> >> >> net/ipv4/ip_gre.c | 14 +++++++++++--- >> >> 1 files changed, 11 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c >> >> index 5ef4da7..0de54cd 100644 >> >> --- a/net/ipv4/ip_gre.c >> >> +++ b/net/ipv4/ip_gre.c >> >> @@ -735,7 +735,8 @@ drop: >> >> return 0; >> >> } >> >> >> >> -static struct sk_buff *handle_offloads(struct sk_buff *skb) >> >> +static struct sk_buff *handle_offloads(struct ip_tunnel *tunnel, >> >> + struct sk_buff *skb) >> >> { >> >> int err; >> >> >> >> @@ -745,8 +746,15 @@ static struct sk_buff *handle_offloads(struct >> >> sk_buff *skb) >> >> goto error; >> >> skb_shinfo(skb)->gso_type |= SKB_GSO_GRE; >> >> return skb; >> >> + } else if (tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM) && >> >> + skb->ip_summed == CHECKSUM_PARTIAL) { >> >> + err = skb_checksum_help(skb); >> >> + if (unlikely(err)) >> >> + goto error; >> >> } >> >> one more thing, there is no need to do csum for GRE_SEQ case. > If csum is not performed TCP csum become incorrect for offload > capable and incapable devices. > > that csum computation is only required for GRE_CSUM case, not for GRE_SEQ. I have send out patches to fix GRE issues, please have a look. > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 5ef4da7..0de54cd 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -735,7 +735,8 @@ drop: return 0; } -static struct sk_buff *handle_offloads(struct sk_buff *skb) +static struct sk_buff *handle_offloads(struct ip_tunnel *tunnel, + struct sk_buff *skb) { int err; @@ -745,8 +746,15 @@ static struct sk_buff *handle_offloads(struct sk_buff *skb) goto error; skb_shinfo(skb)->gso_type |= SKB_GSO_GRE; return skb; + } else if (tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM) && + skb->ip_summed == CHECKSUM_PARTIAL) { + err = skb_checksum_help(skb); + if (unlikely(err)) + goto error; } - if (skb->ip_summed != CHECKSUM_PARTIAL) + + if (skb->ip_summed != CHECKSUM_PARTIAL || + tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM)) skb->ip_summed = CHECKSUM_NONE;