| Message ID | 1484714132.13165.92.camel@edumazet-glaptop3.roam.corp.google.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jan 17, 2017 at 8:35 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Tue, 2017-01-17 at 20:21 -0800, Eric Dumazet wrote: >> On Wed, 2017-01-18 at 03:12 +0000, Ashizuka, Yuusuke wrote: >> >> > indeed. >> > >> > In the case of TSO with i.MX6 system (highmem enabled) with 2GB memory, >> > "this_frag->page.p" did not become highmem area. >> > (We confirmed by transferring about 100MB of files) >> > >> > However, in the case of non-tso on an i.MX6 system with 2GB of memory, >> > "this_frag->page.p" may become a highmem area. >> > (Occurred with approximately 2MB of file transfer) >> > >> > For non-tso only, I do not know the reason why "this_frag-> page.p" >> > in this driver shows highmem area. >> >> This worries me, since this driver does not set NETIF_F_HIGHDMA in its >> features. >> >> No packet should be given to this driver with a highmem fragment >> >> Check is done in illegal_highdma() in net/core/dev.c > > This used to work. > > I suspect commit ec5f061564238892005257c83565a0b58ec79295 > ("net: Kill link between CSUM and SG features.") > > added this bug. > > Can you try this hot fix : > > diff --git a/net/core/dev.c b/net/core/dev.c > index ad5959e561166f445bdd9d7260652a338f74cfea..073b832b945257dba9ed47f4bf875605225effc9 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2773,9 +2773,9 @@ static netdev_features_t harmonize_features(struct sk_buff *skb, > if (skb->ip_summed != CHECKSUM_NONE && > !can_checksum_protocol(features, type)) { > features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); > - } else if (illegal_highdma(skb->dev, skb)) { > - features &= ~NETIF_F_SG; > } > + if (illegal_highdma(skb->dev, skb)) > + features &= ~NETIF_F_SG; > > return features; > } Right, this high mem check should be decoupled from csum check. Thanks, Pravin.
On Wed, 2017-01-18 at 10:18 -0800, Pravin Shelar wrote:
\
> Right, this high mem check should be decoupled from csum check.
I must say I am surprised nobody hit this problem before today.
linux-3.10 is more than 3 years old.
From: Eric Dumazet <eric.dumazet@gmail.com> Sent: Wednesday, January 18, 2017 12:36 PM >To: Ashizuka, Yuusuke <ashiduka@jp.fujitsu.com> >Cc: Andy Duan <fugang.duan@nxp.com>; netdev@vger.kernel.org; Pravin B >Shelar <pshelar@nicira.com> >Subject: Re: [PATCH] net: fec: Fixed panic problem with non-tso > >On Tue, 2017-01-17 at 20:21 -0800, Eric Dumazet wrote: >> On Wed, 2017-01-18 at 03:12 +0000, Ashizuka, Yuusuke wrote: >> >> > indeed. >> > >> > In the case of TSO with i.MX6 system (highmem enabled) with 2GB >> > memory, "this_frag->page.p" did not become highmem area. >> > (We confirmed by transferring about 100MB of files) >> > >> > However, in the case of non-tso on an i.MX6 system with 2GB of >> > memory, "this_frag->page.p" may become a highmem area. >> > (Occurred with approximately 2MB of file transfer) >> > >> > For non-tso only, I do not know the reason why "this_frag-> page.p" >> > in this driver shows highmem area. >> >> This worries me, since this driver does not set NETIF_F_HIGHDMA in its >> features. >> >> No packet should be given to this driver with a highmem fragment >> >> Check is done in illegal_highdma() in net/core/dev.c > >This used to work. > >I suspect commit ec5f061564238892005257c83565a0b58ec79295 >("net: Kill link between CSUM and SG features.") > >added this bug. > >Can you try this hot fix : > >diff --git a/net/core/dev.c b/net/core/dev.c index >ad5959e561166f445bdd9d7260652a338f74cfea..073b832b945257dba9ed47f4bf >875605225effc9 100644 >--- a/net/core/dev.c >+++ b/net/core/dev.c >@@ -2773,9 +2773,9 @@ static netdev_features_t harmonize_features(struct >sk_buff *skb, > if (skb->ip_summed != CHECKSUM_NONE && > !can_checksum_protocol(features, type)) { > features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); >- } else if (illegal_highdma(skb->dev, skb)) { >- features &= ~NETIF_F_SG; > } >+ if (illegal_highdma(skb->dev, skb)) >+ features &= ~NETIF_F_SG; > > return features; > } > > I will double check your fix. Thanks. And, if driver is to support highmem, then we should add tso highmem support in net/core/tso.c, do you think it is necessary ?
On Thu, 2017-01-19 at 08:18 +0000, Andy Duan wrote: > I will double check your fix. Thanks. > > And, if driver is to support highmem, then we should add tso highmem > support in net/core/tso.c, do you think it is necessary ? Adding TSO highmem support would mean changing net/core/tso.c ABI and thus changing all net/core/tso.c users. Looks a lot of work to me.
diff --git a/net/core/dev.c b/net/core/dev.c index ad5959e561166f445bdd9d7260652a338f74cfea..073b832b945257dba9ed47f4bf875605225effc9 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2773,9 +2773,9 @@ static netdev_features_t harmonize_features(struct sk_buff *skb, if (skb->ip_summed != CHECKSUM_NONE && !can_checksum_protocol(features, type)) { features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); - } else if (illegal_highdma(skb->dev, skb)) { - features &= ~NETIF_F_SG; } + if (illegal_highdma(skb->dev, skb)) + features &= ~NETIF_F_SG; return features; }