* Alignment issues with freescale FEC driver @ 2016-09-23 16:43 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 16:43 UTC (permalink / raw) To: linux-arm-kernel, netdev, rmk+kernel, edumazet, Fugang Duan, Troy Kisky Cc: Otavio Salvador, Simone Hello all, We're seeing alignment issues from the ethernet stack on an i.MX6UL board: root@mx6ul:~# cat /proc/cpu/alignment User: 0 System: 470010 (inet_gro_receive+0x104/0x278) This seems to be related to the ip header alignment, and there was much discussion in mailing list threads [1] and [2]. In particular, Russell referred to a patch here, but I haven't been able to find it: https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html Eric Dumazet also suggested a path toward fixing it, but I don't quite understand the suggestion: http://www.spinics.net/lists/netdev/msg213166.htm The immediate problem is addressed by just reading the id and frag_offs fields in the iphdr structure as shown in this patch: commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d Author: Eric Nelson <eric@nelint.com> Date: Fri Sep 23 08:26:03 2016 -0700 net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit value Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807 Signed-off-by: Eric Nelson <eric@nelint.com> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 0cc98b1..c17ef6e 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head, unsigned int hlen; unsigned int off; unsigned int id; + unsigned int frag; int flush = 1; int proto; @@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head, if (unlikely(ip_fast_csum((u8 *)iph, 5))) goto out_unlock; - id = ntohl(*(__be32 *)&iph->id); - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)); - id >>= 16; + id = ntohs(*(__be16 *)&iph->id); + frag = ntohs(*(__be16 *)&iph->frag_off); + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & ~IP_DF)); for (p = *head; p; p = p->next) { struct iphdr *iph2; The reading of both fields in one "ntohl" seems obfuscated at best and certainly worthy of a comment about the optimization but I understand from other notes that the fundamental problem is that the IP header should be aligned on a 4-byte boundary and that's not possible without a memcpy. I'd like to hear suggestions about how we can address this. Regards, Eric [1] - http://www.spinics.net/lists/netdev/msg213114.html [2] - https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002828.html ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 16:43 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 16:43 UTC (permalink / raw) To: linux-arm-kernel Hello all, We're seeing alignment issues from the ethernet stack on an i.MX6UL board: root at mx6ul:~# cat /proc/cpu/alignment User: 0 System: 470010 (inet_gro_receive+0x104/0x278) This seems to be related to the ip header alignment, and there was much discussion in mailing list threads [1] and [2]. In particular, Russell referred to a patch here, but I haven't been able to find it: https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html Eric Dumazet also suggested a path toward fixing it, but I don't quite understand the suggestion: http://www.spinics.net/lists/netdev/msg213166.htm The immediate problem is addressed by just reading the id and frag_offs fields in the iphdr structure as shown in this patch: commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d Author: Eric Nelson <eric@nelint.com> Date: Fri Sep 23 08:26:03 2016 -0700 net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit value Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807 Signed-off-by: Eric Nelson <eric@nelint.com> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 0cc98b1..c17ef6e 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head, unsigned int hlen; unsigned int off; unsigned int id; + unsigned int frag; int flush = 1; int proto; @@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head, if (unlikely(ip_fast_csum((u8 *)iph, 5))) goto out_unlock; - id = ntohl(*(__be32 *)&iph->id); - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)); - id >>= 16; + id = ntohs(*(__be16 *)&iph->id); + frag = ntohs(*(__be16 *)&iph->frag_off); + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & ~IP_DF)); for (p = *head; p; p = p->next) { struct iphdr *iph2; The reading of both fields in one "ntohl" seems obfuscated at best and certainly worthy of a comment about the optimization but I understand from other notes that the fundamental problem is that the IP header should be aligned on a 4-byte boundary and that's not possible without a memcpy. I'd like to hear suggestions about how we can address this. Regards, Eric [1] - http://www.spinics.net/lists/netdev/msg213114.html [2] - https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002828.html ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 16:43 ` Eric Nelson @ 2016-09-23 16:54 ` Eric Dumazet -1 siblings, 0 replies; 55+ messages in thread From: Eric Dumazet @ 2016-09-23 16:54 UTC (permalink / raw) To: Eric Nelson Cc: linux-arm-kernel, netdev, rmk+kernel, Fugang Duan, Troy Kisky, Otavio Salvador, Simone On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote: > > Hello all, > > We're seeing alignment issues from the ethernet stack on an i.MX6UL board: > > root@mx6ul:~# cat /proc/cpu/alignment > User: 0 > System: 470010 (inet_gro_receive+0x104/0x278) > > This seems to be related to the ip header alignment, and there > was much discussion in mailing list threads [1] and [2]. > > In particular, Russell referred to a patch here, but I haven't been > able to find it: > https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html > > Eric Dumazet also suggested a path toward fixing it, but I don't quite > understand the suggestion: > http://www.spinics.net/lists/netdev/msg213166.htm > > The immediate problem is addressed by just reading the id and frag_offs > fields in the iphdr structure as shown in this patch: > > commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d > Author: Eric Nelson <eric@nelint.com> > Date: Fri Sep 23 08:26:03 2016 -0700 > > net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit > value > > Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807 > Signed-off-by: Eric Nelson <eric@nelint.com> > > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c > index 0cc98b1..c17ef6e 100644 > --- a/net/ipv4/af_inet.c > +++ b/net/ipv4/af_inet.c > @@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct > sk_buff **head, > unsigned int hlen; > unsigned int off; > unsigned int id; > + unsigned int frag; > int flush = 1; > int proto; > > @@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct > sk_buff **head, > if (unlikely(ip_fast_csum((u8 *)iph, 5))) > goto out_unlock; > > - id = ntohl(*(__be32 *)&iph->id); > - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)); > - id >>= 16; > + id = ntohs(*(__be16 *)&iph->id); > + frag = ntohs(*(__be16 *)&iph->frag_off); > + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & > ~IP_DF)); > > for (p = *head; p; p = p->next) { > struct iphdr *iph2; > This solves nothing, because a few lines after you'll have yet another unaligned access : ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) | ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) { So you might have one less problematic access, out of hundreds of them all over the places. Really the problem is that whole stack depends on the assumption that IP headers are aligned on arches that care (ie where NET_IP_ALIGN == 2) If your build does have NET_IP_ALIGN = 2 and you get a fault here, it might be because of a buggy driver. The other known case is some GRE encapsulations that break the assumption, and this is discussed somewhere else. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 16:54 ` Eric Dumazet 0 siblings, 0 replies; 55+ messages in thread From: Eric Dumazet @ 2016-09-23 16:54 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote: > > Hello all, > > We're seeing alignment issues from the ethernet stack on an i.MX6UL board: > > root at mx6ul:~# cat /proc/cpu/alignment > User: 0 > System: 470010 (inet_gro_receive+0x104/0x278) > > This seems to be related to the ip header alignment, and there > was much discussion in mailing list threads [1] and [2]. > > In particular, Russell referred to a patch here, but I haven't been > able to find it: > https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html > > Eric Dumazet also suggested a path toward fixing it, but I don't quite > understand the suggestion: > http://www.spinics.net/lists/netdev/msg213166.htm > > The immediate problem is addressed by just reading the id and frag_offs > fields in the iphdr structure as shown in this patch: > > commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d > Author: Eric Nelson <eric@nelint.com> > Date: Fri Sep 23 08:26:03 2016 -0700 > > net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit > value > > Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807 > Signed-off-by: Eric Nelson <eric@nelint.com> > > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c > index 0cc98b1..c17ef6e 100644 > --- a/net/ipv4/af_inet.c > +++ b/net/ipv4/af_inet.c > @@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct > sk_buff **head, > unsigned int hlen; > unsigned int off; > unsigned int id; > + unsigned int frag; > int flush = 1; > int proto; > > @@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct > sk_buff **head, > if (unlikely(ip_fast_csum((u8 *)iph, 5))) > goto out_unlock; > > - id = ntohl(*(__be32 *)&iph->id); > - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)); > - id >>= 16; > + id = ntohs(*(__be16 *)&iph->id); > + frag = ntohs(*(__be16 *)&iph->frag_off); > + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & > ~IP_DF)); > > for (p = *head; p; p = p->next) { > struct iphdr *iph2; > This solves nothing, because a few lines after you'll have yet another unaligned access : ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) | ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) { So you might have one less problematic access, out of hundreds of them all over the places. Really the problem is that whole stack depends on the assumption that IP headers are aligned on arches that care (ie where NET_IP_ALIGN == 2) If your build does have NET_IP_ALIGN = 2 and you get a fault here, it might be because of a buggy driver. The other known case is some GRE encapsulations that break the assumption, and this is discussed somewhere else. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 16:54 ` Eric Dumazet @ 2016-09-23 17:19 ` Eric Nelson -1 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 17:19 UTC (permalink / raw) To: Eric Dumazet Cc: linux-arm-kernel, netdev, rmk+kernel, Fugang Duan, Troy Kisky, Otavio Salvador, Simone Thanks Eric, On 09/23/2016 09:54 AM, Eric Dumazet wrote: > On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote: >> >> Hello all, >> >> We're seeing alignment issues from the ethernet stack on an i.MX6UL board: >> >> <snip> >> >> - id = ntohl(*(__be32 *)&iph->id); >> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)); >> - id >>= 16; >> + id = ntohs(*(__be16 *)&iph->id); >> + frag = ntohs(*(__be16 *)&iph->frag_off); >> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & >> ~IP_DF)); >> >> for (p = *head; p; p = p->next) { >> struct iphdr *iph2; >> > > This solves nothing, because a few lines after you'll have yet another > unaligned access : > Oddly, it does prevent the vast majority (90%+) of the alignment errors. I believe this is because the compiler is generating an ldm instruction when the ntohl() call is used, but I'm stumped about why these aren't generating faults: > ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) | > ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) { > > So you might have one less problematic access, out of hundreds of them > all over the places. > > Really the problem is that whole stack depends on the assumption that > IP headers are aligned on arches that care > (ie where NET_IP_ALIGN == 2) > > If your build does have NET_IP_ALIGN = 2 and you get a fault here, it > might be because of a buggy driver. > NET_IP_ALIGN is set to 2. > The other known case is some GRE encapsulations that break the > assumption, and this is discussed somewhere else. > I don't think that's the case. # CONFIG_IPV6_GRE is not set Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on a 4-byte boundary. Does the ldm instruction require 8-byte alignment? There's definitely a compiler-version dependency involved here, since using gcc 4.9 also reduced the number of faults dramatically. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 17:19 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 17:19 UTC (permalink / raw) To: linux-arm-kernel Thanks Eric, On 09/23/2016 09:54 AM, Eric Dumazet wrote: > On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote: >> >> Hello all, >> >> We're seeing alignment issues from the ethernet stack on an i.MX6UL board: >> >> <snip> >> >> - id = ntohl(*(__be32 *)&iph->id); >> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)); >> - id >>= 16; >> + id = ntohs(*(__be16 *)&iph->id); >> + frag = ntohs(*(__be16 *)&iph->frag_off); >> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & >> ~IP_DF)); >> >> for (p = *head; p; p = p->next) { >> struct iphdr *iph2; >> > > This solves nothing, because a few lines after you'll have yet another > unaligned access : > Oddly, it does prevent the vast majority (90%+) of the alignment errors. I believe this is because the compiler is generating an ldm instruction when the ntohl() call is used, but I'm stumped about why these aren't generating faults: > ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) | > ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) { > > So you might have one less problematic access, out of hundreds of them > all over the places. > > Really the problem is that whole stack depends on the assumption that > IP headers are aligned on arches that care > (ie where NET_IP_ALIGN == 2) > > If your build does have NET_IP_ALIGN = 2 and you get a fault here, it > might be because of a buggy driver. > NET_IP_ALIGN is set to 2. > The other known case is some GRE encapsulations that break the > assumption, and this is discussed somewhere else. > I don't think that's the case. # CONFIG_IPV6_GRE is not set Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on a 4-byte boundary. Does the ldm instruction require 8-byte alignment? There's definitely a compiler-version dependency involved here, since using gcc 4.9 also reduced the number of faults dramatically. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 17:19 ` Eric Nelson @ 2016-09-23 17:33 ` Eric Nelson -1 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 17:33 UTC (permalink / raw) To: Eric Dumazet Cc: linux-arm-kernel, netdev, rmk+kernel, Fugang Duan, Troy Kisky, Otavio Salvador, Simone Hi Eric, On 09/23/2016 10:19 AM, Eric Nelson wrote: > Thanks Eric, > > On 09/23/2016 09:54 AM, Eric Dumazet wrote: >> On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote: >>> >>> Hello all, >>> >>> We're seeing alignment issues from the ethernet stack on an i.MX6UL board: >>> >>> > > <snip> > >>> >>> - id = ntohl(*(__be32 *)&iph->id); >>> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)); >>> - id >>= 16; >>> + id = ntohs(*(__be16 *)&iph->id); >>> + frag = ntohs(*(__be16 *)&iph->frag_off); >>> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & >>> ~IP_DF)); >>> >>> for (p = *head; p; p = p->next) { >>> struct iphdr *iph2; >>> >> >> This solves nothing, because a few lines after you'll have yet another >> unaligned access : >> > > Oddly, it does prevent the vast majority (90%+) of the alignment errors. > > I believe this is because the compiler is generating an ldm instruction > when the ntohl() call is used, but I'm stumped about why these aren't > generating faults: > >> ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) | >> ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) { >> >> So you might have one less problematic access, out of hundreds of them >> all over the places. >> >> Really the problem is that whole stack depends on the assumption that >> IP headers are aligned on arches that care >> (ie where NET_IP_ALIGN == 2) >> >> If your build does have NET_IP_ALIGN = 2 and you get a fault here, it >> might be because of a buggy driver. >> > > NET_IP_ALIGN is set to 2. > >> The other known case is some GRE encapsulations that break the >> assumption, and this is discussed somewhere else. >> > I don't think that's the case. > > # CONFIG_IPV6_GRE is not set > > Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on > a 4-byte boundary. > No. That was wrong. The iphdr is aligned at offsets of 14 from the ethernet frame, which itself is longword aligned. I mistakenly tested before the call to skb_gro_header_slow(), when iph was NULL. After putting a test in the right place, I'm seeing an address of 888a364e for the first un-aligned packet. Since the hardware requires longword alignment for its' DMA transfers, aligning the IP header will require a memcpy, right? You hinted at a solution in this post: http://www.spinics.net/lists/netdev/msg213166.html Are you aware of another driver doing this that could be used as a reference? Please advise, Eric ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 17:33 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 17:33 UTC (permalink / raw) To: linux-arm-kernel Hi Eric, On 09/23/2016 10:19 AM, Eric Nelson wrote: > Thanks Eric, > > On 09/23/2016 09:54 AM, Eric Dumazet wrote: >> On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote: >>> >>> Hello all, >>> >>> We're seeing alignment issues from the ethernet stack on an i.MX6UL board: >>> >>> > > <snip> > >>> >>> - id = ntohl(*(__be32 *)&iph->id); >>> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)); >>> - id >>= 16; >>> + id = ntohs(*(__be16 *)&iph->id); >>> + frag = ntohs(*(__be16 *)&iph->frag_off); >>> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag & >>> ~IP_DF)); >>> >>> for (p = *head; p; p = p->next) { >>> struct iphdr *iph2; >>> >> >> This solves nothing, because a few lines after you'll have yet another >> unaligned access : >> > > Oddly, it does prevent the vast majority (90%+) of the alignment errors. > > I believe this is because the compiler is generating an ldm instruction > when the ntohl() call is used, but I'm stumped about why these aren't > generating faults: > >> ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) | >> ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) { >> >> So you might have one less problematic access, out of hundreds of them >> all over the places. >> >> Really the problem is that whole stack depends on the assumption that >> IP headers are aligned on arches that care >> (ie where NET_IP_ALIGN == 2) >> >> If your build does have NET_IP_ALIGN = 2 and you get a fault here, it >> might be because of a buggy driver. >> > > NET_IP_ALIGN is set to 2. > >> The other known case is some GRE encapsulations that break the >> assumption, and this is discussed somewhere else. >> > I don't think that's the case. > > # CONFIG_IPV6_GRE is not set > > Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on > a 4-byte boundary. > No. That was wrong. The iphdr is aligned at offsets of 14 from the ethernet frame, which itself is longword aligned. I mistakenly tested before the call to skb_gro_header_slow(), when iph was NULL. After putting a test in the right place, I'm seeing an address of 888a364e for the first un-aligned packet. Since the hardware requires longword alignment for its' DMA transfers, aligning the IP header will require a memcpy, right? You hinted at a solution in this post: http://www.spinics.net/lists/netdev/msg213166.html Are you aware of another driver doing this that could be used as a reference? Please advise, Eric ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 17:33 ` Eric Nelson @ 2016-09-23 18:13 ` Andrew Lunn -1 siblings, 0 replies; 55+ messages in thread From: Andrew Lunn @ 2016-09-23 18:13 UTC (permalink / raw) To: Eric Nelson Cc: Eric Dumazet, Fugang Duan, Otavio Salvador, netdev, Troy Kisky, rmk+kernel, Simone, linux-arm-kernel > Since the hardware requires longword alignment for its' DMA transfers, > aligning the IP header will require a memcpy, right? The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts two padding bits on transmit. ENETx_RACC has the same. What about your hardware? Andrew ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 18:13 ` Andrew Lunn 0 siblings, 0 replies; 55+ messages in thread From: Andrew Lunn @ 2016-09-23 18:13 UTC (permalink / raw) To: linux-arm-kernel > Since the hardware requires longword alignment for its' DMA transfers, > aligning the IP header will require a memcpy, right? The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts two padding bits on transmit. ENETx_RACC has the same. What about your hardware? Andrew ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 18:13 ` Andrew Lunn @ 2016-09-23 18:30 ` Russell King - ARM Linux -1 siblings, 0 replies; 55+ messages in thread From: Russell King - ARM Linux @ 2016-09-23 18:30 UTC (permalink / raw) To: Andrew Lunn Cc: Eric Nelson, Eric Dumazet, Fugang Duan, Otavio Salvador, netdev, Troy Kisky, Simone, linux-arm-kernel On Fri, Sep 23, 2016 at 08:13:01PM +0200, Andrew Lunn wrote: > > Since the hardware requires longword alignment for its' DMA transfers, > > aligning the IP header will require a memcpy, right? > > The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts > two padding bits on transmit. ENETx_RACC has the same. > > What about your hardware? The iMX6 FEC also has that ability - as part of my FEC patch stack from ages ago, I implemented support for it. "net:fec: implement almost zero-copy receive path" in my public fec-testing branch. That patch stack is sadly now totally dead and I've no interest in reviving it myself. There was some interest from others in taking my patch stack over, but that went quiet. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 18:30 ` Russell King - ARM Linux 0 siblings, 0 replies; 55+ messages in thread From: Russell King - ARM Linux @ 2016-09-23 18:30 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 23, 2016 at 08:13:01PM +0200, Andrew Lunn wrote: > > Since the hardware requires longword alignment for its' DMA transfers, > > aligning the IP header will require a memcpy, right? > > The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts > two padding bits on transmit. ENETx_RACC has the same. > > What about your hardware? The iMX6 FEC also has that ability - as part of my FEC patch stack from ages ago, I implemented support for it. "net:fec: implement almost zero-copy receive path" in my public fec-testing branch. That patch stack is sadly now totally dead and I've no interest in reviving it myself. There was some interest from others in taking my patch stack over, but that went quiet. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 18:30 ` Russell King - ARM Linux @ 2016-09-23 18:39 ` Eric Nelson -1 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 18:39 UTC (permalink / raw) To: Russell King - ARM Linux, Andrew Lunn Cc: Eric Dumazet, Fugang Duan, Otavio Salvador, netdev, Troy Kisky, Simone, linux-arm-kernel Thanks Russell, On 09/23/2016 11:30 AM, Russell King - ARM Linux wrote: > On Fri, Sep 23, 2016 at 08:13:01PM +0200, Andrew Lunn wrote: >>> Since the hardware requires longword alignment for its' DMA transfers, >>> aligning the IP header will require a memcpy, right? >> >> The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts >> two padding bits on transmit. ENETx_RACC has the same. >> >> What about your hardware? > > The iMX6 FEC also has that ability - as part of my FEC patch stack from > ages ago, I implemented support for it. > > "net:fec: implement almost zero-copy receive path" > > in my public fec-testing branch. > > That patch stack is sadly now totally dead and I've no interest in > reviving it myself. There was some interest from others in taking my > patch stack over, but that went quiet. > I'll take a look and hopefully revive at least part of the patch set. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 18:39 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 18:39 UTC (permalink / raw) To: linux-arm-kernel Thanks Russell, On 09/23/2016 11:30 AM, Russell King - ARM Linux wrote: > On Fri, Sep 23, 2016 at 08:13:01PM +0200, Andrew Lunn wrote: >>> Since the hardware requires longword alignment for its' DMA transfers, >>> aligning the IP header will require a memcpy, right? >> >> The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts >> two padding bits on transmit. ENETx_RACC has the same. >> >> What about your hardware? > > The iMX6 FEC also has that ability - as part of my FEC patch stack from > ages ago, I implemented support for it. > > "net:fec: implement almost zero-copy receive path" > > in my public fec-testing branch. > > That patch stack is sadly now totally dead and I've no interest in > reviving it myself. There was some interest from others in taking my > patch stack over, but that went quiet. > I'll take a look and hopefully revive at least part of the patch set. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 18:13 ` Andrew Lunn @ 2016-09-23 18:35 ` Eric Nelson -1 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 18:35 UTC (permalink / raw) To: Andrew Lunn Cc: Eric Dumazet, Fugang Duan, Otavio Salvador, netdev, Troy Kisky, rmk+kernel, Simone, linux-arm-kernel Thanks Andrew. On 09/23/2016 11:13 AM, Andrew Lunn wrote: >> Since the hardware requires longword alignment for its' DMA transfers, >> aligning the IP header will require a memcpy, right? > > The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts > two padding bits on transmit. ENETx_RACC has the same. > > What about your hardware? > You got me with the RTFM! >From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this: "RX FIFO Shift-16 When this field is set, the actual frame data starts at bit 16 of the first word read from the RX FIFO aligning the Ethernet payload on a 32-bit boundary." Same for the i.MX6UL. I'm not sure what it will take to use this, but it seems to be exactly what we're looking for. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 18:35 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 18:35 UTC (permalink / raw) To: linux-arm-kernel Thanks Andrew. On 09/23/2016 11:13 AM, Andrew Lunn wrote: >> Since the hardware requires longword alignment for its' DMA transfers, >> aligning the IP header will require a memcpy, right? > > The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts > two padding bits on transmit. ENETx_RACC has the same. > > What about your hardware? > You got me with the RTFM! >From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this: "RX FIFO Shift-16 When this field is set, the actual frame data starts at bit 16 of the first word read from the RX FIFO aligning the Ethernet payload on a 32-bit boundary." Same for the i.MX6UL. I'm not sure what it will take to use this, but it seems to be exactly what we're looking for. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 18:35 ` Eric Nelson @ 2016-09-24 2:45 ` David Miller -1 siblings, 0 replies; 55+ messages in thread From: David Miller @ 2016-09-24 2:45 UTC (permalink / raw) To: eric Cc: andrew, edumazet, fugang.duan, otavio, netdev, troy.kisky, rmk+kernel, cjb.sw.nospam, linux-arm-kernel From: Eric Nelson <eric@nelint.com> Date: Fri, 23 Sep 2016 11:35:17 -0700 > From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this: > > "RX FIFO Shift-16 > > When this field is set, the actual frame data starts at bit 16 of the first > word read from the RX FIFO aligning the Ethernet payload on a > 32-bit boundary." > > Same for the i.MX6UL. > > I'm not sure what it will take to use this, but it seems to be exactly > what we're looking for. +1 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-24 2:45 ` David Miller 0 siblings, 0 replies; 55+ messages in thread From: David Miller @ 2016-09-24 2:45 UTC (permalink / raw) To: linux-arm-kernel From: Eric Nelson <eric@nelint.com> Date: Fri, 23 Sep 2016 11:35:17 -0700 > From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this: > > "RX FIFO Shift-16 > > When this field is set, the actual frame data starts at bit 16 of the first > word read from the RX FIFO aligning the Ethernet payload on a > 32-bit boundary." > > Same for the i.MX6UL. > > I'm not sure what it will take to use this, but it seems to be exactly > what we're looking for. +1 ^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: Alignment issues with freescale FEC driver 2016-09-24 2:45 ` David Miller @ 2016-09-24 5:13 ` Andy Duan -1 siblings, 0 replies; 55+ messages in thread From: Andy Duan @ 2016-09-24 5:13 UTC (permalink / raw) To: David Miller, eric Cc: andrew, edumazet, otavio, netdev, troy.kisky, rmk+kernel, cjb.sw.nospam, linux-arm-kernel From: David Miller <davem@davemloft.net> Sent: Saturday, September 24, 2016 10:46 AM > To: eric@nelint.com > Cc: andrew@lunn.ch; edumazet@google.com; Andy Duan > <fugang.duan@nxp.com>; otavio@ossystems.com.br; > netdev@vger.kernel.org; troy.kisky@boundarydevices.com; > rmk+kernel@arm.linux.org.uk; cjb.sw.nospam@gmail.com; linux-arm- > kernel@lists.infradead.org > Subject: Re: Alignment issues with freescale FEC driver > > From: Eric Nelson <eric@nelint.com> > Date: Fri, 23 Sep 2016 11:35:17 -0700 > > > From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this: > > > > "RX FIFO Shift-16 > > > > When this field is set, the actual frame data starts at bit 16 of the > > first word read from the RX FIFO aligning the Ethernet payload on a > > 32-bit boundary." > > > > Same for the i.MX6UL. > > > > I'm not sure what it will take to use this, but it seems to be exactly > > what we're looking for. > > +1 RACC[SHIFT16] just instructs the MAC to write two additional bytes in front of each frame received into the RX FIFO to align the Ethernet payload on a 32-bit boundary. Eric's patch "net: fec: support RRACC_SHIFT16 to align IP header" works fine. For the alignment issues, that is introduced by commit 1b7bde6d6 and c259c132a in net-next tree. Before these commits, no alignment issue. How to fix the issue: Solution1: to enable HW RRACC_SHIFT16 feature (test pass): Eric's patch "net: fec: support RRACC_SHIFT16 to align IP header". Solution2: include the correct prefetch() header (test pass): --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -59,7 +59,7 @@ #include <linux/pinctrl/consumer.h> #include <linux/pm_runtime.h> #include <linux/busfreq-imx.h> -#include <linux/prefetch.h> +#include <asm/processor.h> Solution3: use __netdev_alloc_skb_ip_align() instead of netdev_alloc_skb(). Or: still use the previous method before commit 1b7bde6d6: skb = netdev_alloc_skb(ndev, pkt_len - 4 + NET_IP_ALIGN); skb_reserve(skb, NET_IP_ALIGN); Comparing these solutions: From sw effort and performance, I think these are the similar. Enable RRACC_SHIFT16 doesn't take extra advantage. Correct my if I am wrong. Thanks. Regards, Andy ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-24 5:13 ` Andy Duan 0 siblings, 0 replies; 55+ messages in thread From: Andy Duan @ 2016-09-24 5:13 UTC (permalink / raw) To: linux-arm-kernel From: David Miller <davem@davemloft.net> Sent: Saturday, September 24, 2016 10:46 AM > To: eric at nelint.com > Cc: andrew at lunn.ch; edumazet at google.com; Andy Duan > <fugang.duan@nxp.com>; otavio at ossystems.com.br; > netdev at vger.kernel.org; troy.kisky at boundarydevices.com; > rmk+kernel at arm.linux.org.uk; cjb.sw.nospam at gmail.com; linux-arm- > kernel at lists.infradead.org > Subject: Re: Alignment issues with freescale FEC driver > > From: Eric Nelson <eric@nelint.com> > Date: Fri, 23 Sep 2016 11:35:17 -0700 > > > From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this: > > > > "RX FIFO Shift-16 > > > > When this field is set, the actual frame data starts at bit 16 of the > > first word read from the RX FIFO aligning the Ethernet payload on a > > 32-bit boundary." > > > > Same for the i.MX6UL. > > > > I'm not sure what it will take to use this, but it seems to be exactly > > what we're looking for. > > +1 RACC[SHIFT16] just instructs the MAC to write two additional bytes in front of each frame received into the RX FIFO to align the Ethernet payload on a 32-bit boundary. Eric's patch "net: fec: support RRACC_SHIFT16 to align IP header" works fine. For the alignment issues, that is introduced by commit 1b7bde6d6 and c259c132a in net-next tree. Before these commits, no alignment issue. How to fix the issue: Solution1: to enable HW RRACC_SHIFT16 feature (test pass): Eric's patch "net: fec: support RRACC_SHIFT16 to align IP header". Solution2: include the correct prefetch() header (test pass): --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -59,7 +59,7 @@ #include <linux/pinctrl/consumer.h> #include <linux/pm_runtime.h> #include <linux/busfreq-imx.h> -#include <linux/prefetch.h> +#include <asm/processor.h> Solution3: use __netdev_alloc_skb_ip_align() instead of netdev_alloc_skb(). Or: still use the previous method before commit 1b7bde6d6: skb = netdev_alloc_skb(ndev, pkt_len - 4 + NET_IP_ALIGN); skb_reserve(skb, NET_IP_ALIGN); Comparing these solutions: From sw effort and performance, I think these are the similar. Enable RRACC_SHIFT16 doesn't take extra advantage. Correct my if I am wrong. Thanks. Regards, Andy ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 0/3] net: fec: updates to align IP header 2016-09-24 5:13 ` Andy Duan (?) @ 2016-09-24 14:42 ` Eric Nelson 2016-09-24 14:42 ` [PATCH 1/3] net: fec: remove QUIRK_HAS_RACC from i.mx25 Eric Nelson ` (4 more replies) -1 siblings, 5 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-24 14:42 UTC (permalink / raw) To: netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig, Eric Nelson This patch series is the outcome of investigation into very high numbers of alignment faults on kernel 4.1.33 from the linux-fslc tree: https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx The first two patches remove support for the receive accelerator (RACC) from the i.MX25 and i.MX27 SoCs which don't support the function. The third patch enables hardware alignment of the ethernet packet payload (and especially the IP header) to prevent alignment faults in the IP stack. Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed on the order of 70k alignment faults during a 100MiB transfer using wget. Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed a much more modest improvement from 10's of faults, and it's not clear why that's the case. Eric Nelson (3): net: fec: remove QUIRK_HAS_RACC from i.mx25 net: fec: remove QUIRK_HAS_RACC from i.mx27 net: fec: align IP header in hardware drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 1/3] net: fec: remove QUIRK_HAS_RACC from i.mx25 2016-09-24 14:42 ` [PATCH 0/3] net: fec: updates to align IP header Eric Nelson @ 2016-09-24 14:42 ` Eric Nelson 2016-09-24 14:42 ` [PATCH 2/3] net: fec: remove QUIRK_HAS_RACC from i.mx27 Eric Nelson ` (3 subsequent siblings) 4 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-24 14:42 UTC (permalink / raw) To: netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig, Eric Nelson According to the i.MX25 reference manual, this SoC does not have support for the receive accelerator (RACC) register at offset 0x1C4. http://www.nxp.com/files/dsp/doc/ref_manual/IMX25RM.pdf Signed-off-by: Eric Nelson <eric@nelint.com> --- drivers/net/ethernet/freescale/fec_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index fb5c638..d193406 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -89,7 +89,7 @@ static struct platform_device_id fec_devtype[] = { .driver_data = 0, }, { .name = "imx25-fec", - .driver_data = FEC_QUIRK_USE_GASKET | FEC_QUIRK_HAS_RACC, + .driver_data = FEC_QUIRK_USE_GASKET, }, { .name = "imx27-fec", .driver_data = FEC_QUIRK_HAS_RACC, -- 2.7.4 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH 2/3] net: fec: remove QUIRK_HAS_RACC from i.mx27 2016-09-24 14:42 ` [PATCH 0/3] net: fec: updates to align IP header Eric Nelson 2016-09-24 14:42 ` [PATCH 1/3] net: fec: remove QUIRK_HAS_RACC from i.mx25 Eric Nelson @ 2016-09-24 14:42 ` Eric Nelson 2016-09-24 14:42 ` [PATCH 3/3] net: fec: align IP header in hardware Eric Nelson ` (2 subsequent siblings) 4 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-24 14:42 UTC (permalink / raw) To: netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig, Eric Nelson According to the i.MX27 reference manual, this SoC does not have support for the receive accelerator (RACC) register at offset 0x1C4. http://cache.nxp.com/files/32bit/doc/ref_manual/MCIMX27RM.pdf Signed-off-by: Eric Nelson <eric@nelint.com> --- drivers/net/ethernet/freescale/fec_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index d193406..0219e79 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -92,7 +92,7 @@ static struct platform_device_id fec_devtype[] = { .driver_data = FEC_QUIRK_USE_GASKET, }, { .name = "imx27-fec", - .driver_data = FEC_QUIRK_HAS_RACC, + .driver_data = 0, }, { .name = "imx28-fec", .driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME | -- 2.7.4 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH 3/3] net: fec: align IP header in hardware 2016-09-24 14:42 ` [PATCH 0/3] net: fec: updates to align IP header Eric Nelson 2016-09-24 14:42 ` [PATCH 1/3] net: fec: remove QUIRK_HAS_RACC from i.mx25 Eric Nelson 2016-09-24 14:42 ` [PATCH 2/3] net: fec: remove QUIRK_HAS_RACC from i.mx27 Eric Nelson @ 2016-09-24 14:42 ` Eric Nelson 2016-09-26 9:26 ` David Laight 2016-09-24 15:09 ` [PATCH 0/3] net: fec: updates to align IP header Andy Duan 2016-09-27 11:40 ` David Miller 4 siblings, 1 reply; 55+ messages in thread From: Eric Nelson @ 2016-09-24 14:42 UTC (permalink / raw) To: netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig, Eric Nelson The FEC receive accelerator (RACC) supports shifting the data payload of received packets by 16-bits, which aligns the payload (IP header) on a 4-byte boundary, which is, if not required, at least strongly suggested by the Linux networking layer. Without this patch, a huge number of alignment faults will be taken by the IP stack, as seen in /proc/cpu/alignment: ~/$ cat /proc/cpu/alignment User: 0 System: 72645 (inet_gro_receive+0x104/0x27c) Skipped: 0 Half: 0 Word: 0 DWord: 0 Multi: 72645 User faults: 3 (fixup+warn) This patch was suggested by Andrew Lunn in this message to linux-netdev: http://marc.info/?l=linux-arm-kernel&m=147465452108384&w=2 and adapted from a patch by Russell King from 2014: http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?id=70d8a8a Signed-off-by: Eric Nelson <eric@nelint.com> --- drivers/net/ethernet/freescale/fec_main.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 0219e79..1fa2d87 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -180,6 +180,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); /* FEC receive acceleration */ #define FEC_RACC_IPDIS (1 << 1) #define FEC_RACC_PRODIS (1 << 2) +#define FEC_RACC_SHIFT16 BIT(7) #define FEC_RACC_OPTIONS (FEC_RACC_IPDIS | FEC_RACC_PRODIS) /* @@ -945,9 +946,11 @@ fec_restart(struct net_device *ndev) #if !defined(CONFIG_M5272) if (fep->quirks & FEC_QUIRK_HAS_RACC) { - /* set RX checksum */ val = readl(fep->hwp + FEC_RACC); + /* align IP header */ + val |= FEC_RACC_SHIFT16; if (fep->csum_flags & FLAG_RX_CSUM_ENABLED) + /* set RX checksum */ val |= FEC_RACC_OPTIONS; else val &= ~FEC_RACC_OPTIONS; @@ -1428,6 +1431,12 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id) prefetch(skb->data - NET_IP_ALIGN); skb_put(skb, pkt_len - 4); data = skb->data; + +#if !defined(CONFIG_M5272) + if (fep->quirks & FEC_QUIRK_HAS_RACC) + data = skb_pull_inline(skb, 2); +#endif + if (!is_copybreak && need_swap) swap_buffer(data, pkt_len); -- 2.7.4 ^ permalink raw reply related [flat|nested] 55+ messages in thread
* RE: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-24 14:42 ` [PATCH 3/3] net: fec: align IP header in hardware Eric Nelson @ 2016-09-26 9:26 ` David Laight 2016-09-26 18:39 ` Eric Nelson 0 siblings, 1 reply; 55+ messages in thread From: David Laight @ 2016-09-26 9:26 UTC (permalink / raw) To: 'Eric Nelson', netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig From: Eric Nelson > Sent: 24 September 2016 15:42 > The FEC receive accelerator (RACC) supports shifting the data payload of > received packets by 16-bits, which aligns the payload (IP header) on a > 4-byte boundary, which is, if not required, at least strongly suggested > by the Linux networking layer. ... > + /* align IP header */ > + val |= FEC_RACC_SHIFT16; I can't help feeling that there needs to be corresponding changes to increase the buffer size by 2 (maybe for large mtu) and to discard two bytes from the frame length. If probably ought to be predicated on NET_IP_ALIGN as well. David ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-26 9:26 ` David Laight @ 2016-09-26 18:39 ` Eric Nelson 2016-09-28 16:42 ` David Laight 0 siblings, 1 reply; 55+ messages in thread From: Eric Nelson @ 2016-09-26 18:39 UTC (permalink / raw) To: David Laight, netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig Hi David, On 09/26/2016 02:26 AM, David Laight wrote: > From: Eric Nelson >> Sent: 24 September 2016 15:42 >> The FEC receive accelerator (RACC) supports shifting the data payload of >> received packets by 16-bits, which aligns the payload (IP header) on a >> 4-byte boundary, which is, if not required, at least strongly suggested >> by the Linux networking layer. > ... >> + /* align IP header */ >> + val |= FEC_RACC_SHIFT16; > > I can't help feeling that there needs to be corresponding > changes to increase the buffer size by 2 (maybe for large mtu) > and to discard two bytes from the frame length. > In the normal case, the fec driver over-allocates all receive packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, which is either 0x0f (ARM) or 0x03 (PPC). If the frame length is less than rx_copybreak (typically 256), then the frame length from the receive buffer descriptor is used to control the allocation size for a copied buffer, and this will include the two bytes of padding if RACC_SHIFT16 is set. > If probably ought to be predicated on NET_IP_ALIGN as well. > Can you elaborate? Please advise, Eric ^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-26 18:39 ` Eric Nelson @ 2016-09-28 16:42 ` David Laight 2016-09-28 17:14 ` Eric Nelson 0 siblings, 1 reply; 55+ messages in thread From: David Laight @ 2016-09-28 16:42 UTC (permalink / raw) To: 'Eric Nelson', netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig From: Eric Nelson > Sent: 26 September 2016 19:40 > Hi David, > > On 09/26/2016 02:26 AM, David Laight wrote: > > From: Eric Nelson > >> Sent: 24 September 2016 15:42 > >> The FEC receive accelerator (RACC) supports shifting the data payload of > >> received packets by 16-bits, which aligns the payload (IP header) on a > >> 4-byte boundary, which is, if not required, at least strongly suggested > >> by the Linux networking layer. > > ... > >> + /* align IP header */ > >> + val |= FEC_RACC_SHIFT16; > > > > I can't help feeling that there needs to be corresponding > > changes to increase the buffer size by 2 (maybe for large mtu) > > and to discard two bytes from the frame length. > > > > In the normal case, the fec driver over-allocates all receive packets to > be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, > which is either 0x0f (ARM) or 0x03 (PPC). > > If the frame length is less than rx_copybreak (typically 256), then > the frame length from the receive buffer descriptor is used to > control the allocation size for a copied buffer, and this will include > the two bytes of padding if RACC_SHIFT16 is set. > > > If probably ought to be predicated on NET_IP_ALIGN as well. > > > Can you elaborate? >From reading this it seems that the effect of FEC_RACC_SHIFT16 is to add two bytes of 'junk' to the start of every receive frame. In the 'copybreak' case the new skb would need to be 2 bytes shorter than the length reported by the hardware, and the data copied from 2 bytes into the dma buffer. The extra 2 bytes also mean the that maximum mtu that can be received into a buffer is two bytes less. If someone sets the mtu to (say) 9k for jumbo frames this might matter. Even with fixed 2048 byte buffers it reduces the maximum value the mtu can be set to by 2. Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start on a 4n boundary, and the skb are likely to be allocated that way. In this case you don't want to extra two bytes of 'junk'. OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that the data is dma'd to offset -2 in the skb and then ensure that the end of frame is set correctly. David ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-28 16:42 ` David Laight @ 2016-09-28 17:14 ` Eric Nelson 2016-09-28 17:25 ` Russell King - ARM Linux 2016-09-29 11:07 ` David Laight 0 siblings, 2 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-28 17:14 UTC (permalink / raw) To: David Laight, netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig Thanks David, On 09/28/2016 09:42 AM, David Laight wrote: > From: Eric Nelson >> Sent: 26 September 2016 19:40 >> Hi David, >> >> On 09/26/2016 02:26 AM, David Laight wrote: >>> From: Eric Nelson >>>> Sent: 24 September 2016 15:42 >>>> The FEC receive accelerator (RACC) supports shifting the data payload of >>>> received packets by 16-bits, which aligns the payload (IP header) on a >>>> 4-byte boundary, which is, if not required, at least strongly suggested >>>> by the Linux networking layer. >>> ... >>>> + /* align IP header */ >>>> + val |= FEC_RACC_SHIFT16; >>> >>> I can't help feeling that there needs to be corresponding >>> changes to increase the buffer size by 2 (maybe for large mtu) >>> and to discard two bytes from the frame length. >>> >> >> In the normal case, the fec driver over-allocates all receive packets to >> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, >> which is either 0x0f (ARM) or 0x03 (PPC). >> >> If the frame length is less than rx_copybreak (typically 256), then >> the frame length from the receive buffer descriptor is used to >> control the allocation size for a copied buffer, and this will include >> the two bytes of padding if RACC_SHIFT16 is set. >> >>> If probably ought to be predicated on NET_IP_ALIGN as well. >>> >> Can you elaborate? > > From reading this it seems that the effect of FEC_RACC_SHIFT16 is to > add two bytes of 'junk' to the start of every receive frame. > That's right. Two bytes of junk between the MAC header and the IP header. > In the 'copybreak' case the new skb would need to be 2 bytes shorter > than the length reported by the hardware, and the data copied from > 2 bytes into the dma buffer. > As it stands, the skb allocated by the copybreak routine will include the two bytes of padding, and the call to skb_pull_inline will ignore them. > The extra 2 bytes also mean the that maximum mtu that can be received > into a buffer is two bytes less. > Right, but I think the max is already high enough that this isn't a problem. > If someone sets the mtu to (say) 9k for jumbo frames this might matter. > Even with fixed 2048 byte buffers it reduces the maximum value the mtu > can be set to by 2. > As far as I can tell, the fec driver doesn't support jumbo frames, and the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522). This is well within the 2048-byte allocation, even with optional headers for VLAN etc. > Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start > on a 4n boundary, and the skb are likely to be allocated that way. > In this case you don't want to extra two bytes of 'junk'. > NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h > OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that > the data is dma'd to offset -2 in the skb and then ensure that the > end of frame is set correctly. > That's what the RACC SHIFT16 bit does. The FEC hardware isn't capable of DMA'ing to an un-aligned address. On ARM, it requires 64-bit alignment, but suggests 128-bit alignment. On other (PPC?) architectures, it requires 32-bit alignment. This is handled by the rx_align field. Regards, Eric ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-28 17:14 ` Eric Nelson @ 2016-09-28 17:25 ` Russell King - ARM Linux 2016-09-28 18:01 ` Eric Nelson 2016-09-29 11:07 ` David Laight 1 sibling, 1 reply; 55+ messages in thread From: Russell King - ARM Linux @ 2016-09-28 17:25 UTC (permalink / raw) To: Eric Nelson Cc: David Laight, netdev, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig On Wed, Sep 28, 2016 at 10:14:52AM -0700, Eric Nelson wrote: > Thanks David, > > On 09/28/2016 09:42 AM, David Laight wrote: > > From reading this it seems that the effect of FEC_RACC_SHIFT16 is to > > add two bytes of 'junk' to the start of every receive frame. > > That's right. Two bytes of junk between the MAC header and the > IP header. That's wrong. FEC_RACC_SHIFT16 adds two bytes to the _beginning_ of the packet, not in the middle of the packet: 7 RX FIFO Shift-16 SHIFT16 When this field is set, the actual frame data starts at bit 16 of the first word read from the RX FIFO aligning the Ethernet payload on a 32-bit boundary. NOTE: This function only affects the FIFO storage and has no influence on the statistics, which use the actual length of the frame received. 0 Disabled. 1 Instructs the MAC to write two additional bytes in front of each frame received into the RX FIFO. *in front* of the frame - that's before the Ethernet header. Not between the ethernet and IP headers. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-28 17:25 ` Russell King - ARM Linux @ 2016-09-28 18:01 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-28 18:01 UTC (permalink / raw) To: Russell King - ARM Linux Cc: David Laight, netdev, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig Thanks Russell, On 09/28/2016 10:25 AM, Russell King - ARM Linux wrote: > On Wed, Sep 28, 2016 at 10:14:52AM -0700, Eric Nelson wrote: >> Thanks David, >> >> On 09/28/2016 09:42 AM, David Laight wrote: >>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to >>> add two bytes of 'junk' to the start of every receive frame. >> >> That's right. Two bytes of junk between the MAC header and the >> IP header. > > That's wrong. FEC_RACC_SHIFT16 adds two bytes to the _beginning_ of > the packet, not in the middle of the packet: > > 7 RX FIFO Shift-16 > SHIFT16 > When this field is set, the actual frame data starts at bit 16 > of the first word read from the RX FIFO aligning the Ethernet > payload on a 32-bit boundary. > > NOTE: This function only affects the FIFO storage and has no > influence on the statistics, which use the actual length > of the frame received. > > 0 Disabled. > 1 Instructs the MAC to write two additional bytes in front > of each frame received into the RX FIFO. > > *in front* of the frame - that's before the Ethernet header. Not between > the ethernet and IP headers. > I obviously mis-read this, and haven't dumped any packets to straighten myself out. ^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-28 17:14 ` Eric Nelson 2016-09-28 17:25 ` Russell King - ARM Linux @ 2016-09-29 11:07 ` David Laight 2016-09-30 13:27 ` Eric Nelson 1 sibling, 1 reply; 55+ messages in thread From: David Laight @ 2016-09-29 11:07 UTC (permalink / raw) To: 'Eric Nelson', netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig From: Eric Nelson > Sent: 28 September 2016 18:15 > On 09/28/2016 09:42 AM, David Laight wrote: > > From: Eric Nelson > >> Sent: 26 September 2016 19:40 > >> Hi David, > >> > >> On 09/26/2016 02:26 AM, David Laight wrote: > >>> From: Eric Nelson > >>>> Sent: 24 September 2016 15:42 > >>>> The FEC receive accelerator (RACC) supports shifting the data payload of > >>>> received packets by 16-bits, which aligns the payload (IP header) on a > >>>> 4-byte boundary, which is, if not required, at least strongly suggested > >>>> by the Linux networking layer. > >>> ... > >>>> + /* align IP header */ > >>>> + val |= FEC_RACC_SHIFT16; > >>> > >>> I can't help feeling that there needs to be corresponding > >>> changes to increase the buffer size by 2 (maybe for large mtu) > >>> and to discard two bytes from the frame length. > >>> > >> > >> In the normal case, the fec driver over-allocates all receive packets to > >> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, > >> which is either 0x0f (ARM) or 0x03 (PPC). > >> > >> If the frame length is less than rx_copybreak (typically 256), then > >> the frame length from the receive buffer descriptor is used to > >> control the allocation size for a copied buffer, and this will include > >> the two bytes of padding if RACC_SHIFT16 is set. > >> > >>> If probably ought to be predicated on NET_IP_ALIGN as well. > >>> > >> Can you elaborate? > > > > From reading this it seems that the effect of FEC_RACC_SHIFT16 is to > > add two bytes of 'junk' to the start of every receive frame. > > > > That's right. Two bytes of junk between the MAC header and the > IP header. > > > In the 'copybreak' case the new skb would need to be 2 bytes shorter > > than the length reported by the hardware, and the data copied from > > 2 bytes into the dma buffer. > > > > As it stands, the skb allocated by the copybreak routine will include > the two bytes of padding, and the call to skb_pull_inline will ignore > them. Ok, I didn't see that call being added by this patch. > > The extra 2 bytes also mean the that maximum mtu that can be received > > into a buffer is two bytes less. > > > > Right, but I think the max is already high enough that this isn't a > problem. > > > If someone sets the mtu to (say) 9k for jumbo frames this might matter. > > Even with fixed 2048 byte buffers it reduces the maximum value the mtu > > can be set to by 2. > > > > As far as I can tell, the fec driver doesn't support jumbo frames, and > the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522). > > This is well within the 2048-byte allocation, even with optional headers > for VLAN etc. Hmm... That (probably) means all the skb the driver allocates are actually 4k. It would be much better to reduce the size so that the entire skb (with packet buffer) is less than 2k. > > Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start > > on a 4n boundary, and the skb are likely to be allocated that way. > > In this case you don't want to extra two bytes of 'junk'. > > > NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h Even though it is always currently set is isn't really ideal to have a driver that breaks if it isn't set. This could easily happen at some point in the future if the ethernet logic is put with a different cpu. > > OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that > > the data is dma'd to offset -2 in the skb and then ensure that the > > end of frame is set correctly. > > > > That's what the RACC SHIFT16 bit does. No, that causes the ethernet controller to add 2 bytes to the frame. You then need to change the dma target address to match. Otherwise if a new version of the silicon stops ignoring the low address with the frame will be misaligned in the buffer. The receive frame length will also (probably) be 2 larger than the actual frame - so you need to set the end point correctly as well. IP will probably ignore the 2 bytes of pad I think you are generating. > The FEC hardware isn't capable of DMA'ing to an un-aligned address. > On ARM, it requires 64-bit alignment, but suggests 128-bit alignment. > > On other (PPC?) architectures, it requires 32-bit alignment. This is > handled by the rx_align field. That isn't entirely relevant. If the kernel is being built with NET_IP_ALIGN set to 0 you should align the destination mac address on a 4n boundary (Or rather the skb are likely to be allocated that way). If it causes misaligned memory reads later on that is a different problem. The MAC driver has aligned the frames as it was told to. David ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-29 11:07 ` David Laight @ 2016-09-30 13:27 ` Eric Nelson 2016-09-30 13:49 ` David Laight 2016-10-08 2:44 ` Andy Duan 0 siblings, 2 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-30 13:27 UTC (permalink / raw) To: David Laight, netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig Thanks for the feedback David, On 09/29/2016 04:07 AM, David Laight wrote: > From: Eric Nelson >> Sent: 28 September 2016 18:15 >> On 09/28/2016 09:42 AM, David Laight wrote: >>> From: Eric Nelson >>>> Sent: 26 September 2016 19:40 >>>> Hi David, >>>> >>>> On 09/26/2016 02:26 AM, David Laight wrote: >>>>> From: Eric Nelson >>>>>> Sent: 24 September 2016 15:42 >>>>>> The FEC receive accelerator (RACC) supports shifting the data payload of >>>>>> received packets by 16-bits, which aligns the payload (IP header) on a >>>>>> 4-byte boundary, which is, if not required, at least strongly suggested >>>>>> by the Linux networking layer. >>>>> ... >>>>>> + /* align IP header */ >>>>>> + val |= FEC_RACC_SHIFT16; >>>>> >>>>> I can't help feeling that there needs to be corresponding >>>>> changes to increase the buffer size by 2 (maybe for large mtu) >>>>> and to discard two bytes from the frame length. >>>>> >>>> >>>> In the normal case, the fec driver over-allocates all receive packets to >>>> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, >>>> which is either 0x0f (ARM) or 0x03 (PPC). >>>> >>>> If the frame length is less than rx_copybreak (typically 256), then >>>> the frame length from the receive buffer descriptor is used to >>>> control the allocation size for a copied buffer, and this will include >>>> the two bytes of padding if RACC_SHIFT16 is set. >>>> >>>>> If probably ought to be predicated on NET_IP_ALIGN as well. >>>>> >>>> Can you elaborate? >>> >>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to >>> add two bytes of 'junk' to the start of every receive frame. >>> >> >> That's right. Two bytes of junk between the MAC header and the >> IP header. >> >>> In the 'copybreak' case the new skb would need to be 2 bytes shorter >>> than the length reported by the hardware, and the data copied from >>> 2 bytes into the dma buffer. >>> >> >> As it stands, the skb allocated by the copybreak routine will include >> the two bytes of padding, and the call to skb_pull_inline will ignore >> them. > > Ok, I didn't see that call being added by this patch. > >>> The extra 2 bytes also mean the that maximum mtu that can be received >>> into a buffer is two bytes less. >>> >> >> Right, but I think the max is already high enough that this isn't a >> problem. >> >>> If someone sets the mtu to (say) 9k for jumbo frames this might matter. >>> Even with fixed 2048 byte buffers it reduces the maximum value the mtu >>> can be set to by 2. >>> >> >> As far as I can tell, the fec driver doesn't support jumbo frames, and >> the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522). >> >> This is well within the 2048-byte allocation, even with optional headers >> for VLAN etc. > > Hmm... > > That (probably) means all the skb the driver allocates are actually 4k. > It would be much better to reduce the size so that the entire skb > (with packet buffer) is less than 2k. > That seems worthwhile, but un-related to this patch. It appears to me that the received packets could be allocated as PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN (+2 if FEC_RACC_SHIFT16 is used) >>> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start >>> on a 4n boundary, and the skb are likely to be allocated that way. >>> In this case you don't want to extra two bytes of 'junk'. >>> >> NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h > > Even though it is always currently set is isn't really ideal to have > a driver that breaks if it isn't set. > This could easily happen at some point in the future if the ethernet > logic is put with a different cpu. > After multiple reads, I'm confused about the meaning of NET_IP_ALIGN and how it should be used. >From Documentation/unaligned-memory-access.txt, I take it that this should be configured on a per-architecture basis, and it seems to be set to zero on both PPC and x86. I wonder if this is proper though. It seems that its' use might depend on the I/O subsystem(s) in use as much as the architecture. For example, it might be desirable to have a different value for a PCIe interface than for an integrated MAC like the FEC. Looking at the example of the 3c59x driver, I see a pattern of an allocation that adds NET_IP_ALIGN followed by an skb->reserve() of NET_IP_ALIGN before determining the target address to end up with allocation with 4n+2 alignment. This seems somewhat equivalent to this patch, except that we're using the allocated address as the target and using skb_pull_inline afterwards. Andy, is the FEC used on any PPC SOCs? If so, then this patch may cause a DMA of 2 extra bytes per frame unecessarily although the driver doesn't special-case the allocation to align the IP header, so this is still probably preferred. >>> OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that >>> the data is dma'd to offset -2 in the skb and then ensure that the >>> end of frame is set correctly. >>> >> >> That's what the RACC SHIFT16 bit does. > > No, that causes the ethernet controller to add 2 bytes to the frame. > You then need to change the dma target address to match. > Or use skb_pull_inline to ignore the two bytes. > Otherwise if a new version of the silicon stops ignoring the low > address with the frame will be misaligned in the buffer. > I'm not sure I understand this. > The receive frame length will also (probably) be 2 larger than the > actual frame - so you need to set the end point correctly as well. > IP will probably ignore the 2 bytes of pad I think you are generating. > The received frame length **is** 2 bytes longer, but these are eaten by skb_pull_inline(). >> The FEC hardware isn't capable of DMA'ing to an un-aligned address. >> On ARM, it requires 64-bit alignment, but suggests 128-bit alignment. >> >> On other (PPC?) architectures, it requires 32-bit alignment. This is >> handled by the rx_align field. > > That isn't entirely relevant. > > If the kernel is being built with NET_IP_ALIGN set to 0 you should > align the destination mac address on a 4n boundary > (Or rather the skb are likely to be allocated that way). They're not currently allocated that way. The routine fec_enet_alloc_rxq_buffers forces the allocations to 32 or 128-bit alignment through the routine fec_enet_new_rxbdp(). > If it causes misaligned memory reads later on that is a different problem. That's the problem this patch is designed to address. Without this patch, the IP header is always mis-aligned. > The MAC driver has aligned the frames as it was told to. > > David > > Regards, Eric ^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-30 13:27 ` Eric Nelson @ 2016-09-30 13:49 ` David Laight 2016-09-30 14:16 ` Eric Nelson 2016-10-08 2:44 ` Andy Duan 1 sibling, 1 reply; 55+ messages in thread From: David Laight @ 2016-09-30 13:49 UTC (permalink / raw) To: 'Eric Nelson', netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig From: Eric Nelson > Sent: 30 September 2016 14:27 > Thanks for the feedback David, > > On 09/29/2016 04:07 AM, David Laight wrote: > > From: Eric Nelson > >> Sent: 28 September 2016 18:15 > >> On 09/28/2016 09:42 AM, David Laight wrote: > >>> From: Eric Nelson > >>>> Sent: 26 September 2016 19:40 > >>>> Hi David, > >>>> > >>>> On 09/26/2016 02:26 AM, David Laight wrote: > >>>>> From: Eric Nelson > >>>>>> Sent: 24 September 2016 15:42 > >>>>>> The FEC receive accelerator (RACC) supports shifting the data payload of > >>>>>> received packets by 16-bits, which aligns the payload (IP header) on a > >>>>>> 4-byte boundary, which is, if not required, at least strongly suggested > >>>>>> by the Linux networking layer. > >>>>> ... > >>>>>> + /* align IP header */ > >>>>>> + val |= FEC_RACC_SHIFT16; > >>>>> > >>>>> I can't help feeling that there needs to be corresponding > >>>>> changes to increase the buffer size by 2 (maybe for large mtu) > >>>>> and to discard two bytes from the frame length. > >>>>> > >>>> > >>>> In the normal case, the fec driver over-allocates all receive packets to > >>>> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, > >>>> which is either 0x0f (ARM) or 0x03 (PPC). > >>>> > >>>> If the frame length is less than rx_copybreak (typically 256), then > >>>> the frame length from the receive buffer descriptor is used to > >>>> control the allocation size for a copied buffer, and this will include > >>>> the two bytes of padding if RACC_SHIFT16 is set. > >>>> > >>>>> If probably ought to be predicated on NET_IP_ALIGN as well. > >>>>> > >>>> Can you elaborate? > >>> > >>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to > >>> add two bytes of 'junk' to the start of every receive frame. > >>> > >> > >> That's right. Two bytes of junk between the MAC header and the > >> IP header. > >> > >>> In the 'copybreak' case the new skb would need to be 2 bytes shorter > >>> than the length reported by the hardware, and the data copied from > >>> 2 bytes into the dma buffer. > >>> > >> > >> As it stands, the skb allocated by the copybreak routine will include > >> the two bytes of padding, and the call to skb_pull_inline will ignore > >> them. > > > > Ok, I didn't see that call being added by this patch. > > > >>> The extra 2 bytes also mean the that maximum mtu that can be received > >>> into a buffer is two bytes less. > >>> > >> > >> Right, but I think the max is already high enough that this isn't a > >> problem. > >> > >>> If someone sets the mtu to (say) 9k for jumbo frames this might matter. > >>> Even with fixed 2048 byte buffers it reduces the maximum value the mtu > >>> can be set to by 2. > >>> > >> > >> As far as I can tell, the fec driver doesn't support jumbo frames, and > >> the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522). > >> > >> This is well within the 2048-byte allocation, even with optional headers > >> for VLAN etc. > > > > Hmm... > > > > That (probably) means all the skb the driver allocates are actually 4k. > > It would be much better to reduce the size so that the entire skb > > (with packet buffer) is less than 2k. > > > > That seems worthwhile, but un-related to this patch. Indeed. > It appears to me that the received packets could be allocated as > > PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN > > (+2 if FEC_RACC_SHIFT16 is used) No. The packet buffers need to be allocated NET_IP_ALIGN + PKT_MAXBUF_SIZE byte long and (I assume) aligned on a fep->rx_align byte boundary. If NET_IP_ALIGN is set (to 2) then FEC_RACC_SHIFT16 must also me set so that the ethernet frame itself is 4n+2 aligned. > >>> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start > >>> on a 4n boundary, and the skb are likely to be allocated that way. > >>> In this case you don't want to extra two bytes of 'junk'. > >>> > >> NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h > > > > Even though it is always currently set is isn't really ideal to have > > a driver that breaks if it isn't set. > > This could easily happen at some point in the future if the ethernet > > logic is put with a different cpu. > > > > After multiple reads, I'm confused about the meaning of NET_IP_ALIGN > and how it should be used. > > From Documentation/unaligned-memory-access.txt, I take it that this > should be configured on a per-architecture basis, and it seems to be > set to zero on both PPC and x86. > > I wonder if this is proper though. It seems that its' use might depend > on the I/O subsystem(s) in use as much as the architecture. ... If the cpu cannot do misaligned memory cycles then NET_IP_ALIGN must be 2 and all receive frames must be aligned like that. If the cpu can do misaligned memory cycles then the alignment of receive ethernet frames doesn't matter that much. NET_IP_ALIGN is likely to be set to zero because the cost of the cpu doing misaligned transfers it likely to be a lot less than that of un-optimised dma accesses to misaligned memory [1] [2]. If NET_IP_ALIGN is zero then I believe that ethernet drivers are allowed to build skb that have the frame on a 4n+2 alignment. This is probably sensible if the hardware can write the two bytes. (DM might correct me there.) David [1] The original sparc sbus 'DMA' part did multiple 16bit transfers instead of a burst of 32bit transfers. This meant the buffer had to be misaligned and a software copy done to align the frames. Fixed in the DMA+ part. [2] PCIe writes are likely to be much faster if they contain entire cache lines of data. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-30 13:49 ` David Laight @ 2016-09-30 14:16 ` Eric Nelson 2016-10-01 19:52 ` Russell King - ARM Linux 0 siblings, 1 reply; 55+ messages in thread From: Eric Nelson @ 2016-09-30 14:16 UTC (permalink / raw) To: David Laight, netdev Cc: linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig Hi David, On 09/30/2016 06:49 AM, David Laight wrote: > From: Eric Nelson >> Sent: 30 September 2016 14:27 >> Thanks for the feedback David, >> >> On 09/29/2016 04:07 AM, David Laight wrote: >>> From: Eric Nelson >>>> Sent: 28 September 2016 18:15 >>>> On 09/28/2016 09:42 AM, David Laight wrote: >>>>> From: Eric Nelson >>>>>> Sent: 26 September 2016 19:40 >>>>>> Hi David, >>>>>> >>>>>> On 09/26/2016 02:26 AM, David Laight wrote: >>>>>>> From: Eric Nelson >>>>>>>> Sent: 24 September 2016 15:42 >>>>>>>> The FEC receive accelerator (RACC) supports shifting the data payload of >>>>>>>> received packets by 16-bits, which aligns the payload (IP header) on a >>>>>>>> 4-byte boundary, which is, if not required, at least strongly suggested >>>>>>>> by the Linux networking layer. >>>>>>> ... >>>>>>>> + /* align IP header */ >>>>>>>> + val |= FEC_RACC_SHIFT16; >>>>>>> >>>>>>> I can't help feeling that there needs to be corresponding >>>>>>> changes to increase the buffer size by 2 (maybe for large mtu) >>>>>>> and to discard two bytes from the frame length. >>>>>>> >>>>>> >>>>>> In the normal case, the fec driver over-allocates all receive packets to >>>>>> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align, >>>>>> which is either 0x0f (ARM) or 0x03 (PPC). >>>>>> >>>>>> If the frame length is less than rx_copybreak (typically 256), then >>>>>> the frame length from the receive buffer descriptor is used to >>>>>> control the allocation size for a copied buffer, and this will include >>>>>> the two bytes of padding if RACC_SHIFT16 is set. >>>>>> >>>>>>> If probably ought to be predicated on NET_IP_ALIGN as well. >>>>>>> >>>>>> Can you elaborate? >>>>> >>>>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to >>>>> add two bytes of 'junk' to the start of every receive frame. >>>>> >>>> >>>> That's right. Two bytes of junk between the MAC header and the >>>> IP header. >>>> >>>>> In the 'copybreak' case the new skb would need to be 2 bytes shorter >>>>> than the length reported by the hardware, and the data copied from >>>>> 2 bytes into the dma buffer. >>>>> >>>> >>>> As it stands, the skb allocated by the copybreak routine will include >>>> the two bytes of padding, and the call to skb_pull_inline will ignore >>>> them. >>> >>> Ok, I didn't see that call being added by this patch. >>> >>>>> The extra 2 bytes also mean the that maximum mtu that can be received >>>>> into a buffer is two bytes less. >>>>> >>>> >>>> Right, but I think the max is already high enough that this isn't a >>>> problem. >>>> >>>>> If someone sets the mtu to (say) 9k for jumbo frames this might matter. >>>>> Even with fixed 2048 byte buffers it reduces the maximum value the mtu >>>>> can be set to by 2. >>>>> >>>> >>>> As far as I can tell, the fec driver doesn't support jumbo frames, and >>>> the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522). >>>> >>>> This is well within the 2048-byte allocation, even with optional headers >>>> for VLAN etc. >>> >>> Hmm... >>> >>> That (probably) means all the skb the driver allocates are actually 4k. >>> It would be much better to reduce the size so that the entire skb >>> (with packet buffer) is less than 2k. >>> >> >> That seems worthwhile, but un-related to this patch. > > Indeed. > >> It appears to me that the received packets could be allocated as >> >> PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN >> >> (+2 if FEC_RACC_SHIFT16 is used) > > No. > The packet buffers need to be allocated NET_IP_ALIGN + PKT_MAXBUF_SIZE > byte long and (I assume) aligned on a fep->rx_align byte boundary. > I think we're saying the same thing here, with the exception of the +2 for FEC_RACC_SHIFT16. > If NET_IP_ALIGN is set (to 2) then FEC_RACC_SHIFT16 must also me set > so that the ethernet frame itself is 4n+2 aligned. > This patch does this, but not with the beginning of the skb. It also does this when NET_IP_ALIGN is zero though, and I believe this is the right thing, so the IP header is aligned in a sensible way. The driver can't handle a DMA to (4n+2) on any architecture. >>>>> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start >>>>> on a 4n boundary, and the skb are likely to be allocated that way. >>>>> In this case you don't want to extra two bytes of 'junk'. >>>>> >>>> NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h >>> >>> Even though it is always currently set is isn't really ideal to have >>> a driver that breaks if it isn't set. >>> This could easily happen at some point in the future if the ethernet >>> logic is put with a different cpu. >>> >> >> After multiple reads, I'm confused about the meaning of NET_IP_ALIGN >> and how it should be used. >> >> From Documentation/unaligned-memory-access.txt, I take it that this >> should be configured on a per-architecture basis, and it seems to be >> set to zero on both PPC and x86. >> >> I wonder if this is proper though. It seems that its' use might depend >> on the I/O subsystem(s) in use as much as the architecture. > ... > > If the cpu cannot do misaligned memory cycles then NET_IP_ALIGN must be 2 > and all receive frames must be aligned like that. > On ARM, the CPU can't handle misaligned memory cycles without taking an alignment fault and NET_IP_ALIGN is set to 2. On PPC, NET_IP_ALIGN is set to zero. I could use some help from NXP about whether the driver is used on PPC, but I don't think it can DMA to 4n+2 addresses on any architecture and the purpose of this patch is to align the frame on a (4n+2) address. > If the cpu can do misaligned memory cycles then the alignment of receive > ethernet frames doesn't matter that much. > NET_IP_ALIGN is likely to be set to zero because the cost of the cpu > doing misaligned transfers it likely to be a lot less than that of > un-optimised dma accesses to misaligned memory [1] [2]. > On ARM, we have CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y but I find it hard to believe that taking alignment faults is more efficient than adding two bytes to the start of the frame. > If NET_IP_ALIGN is zero then I believe that ethernet drivers are > allowed to build skb that have the frame on a 4n+2 alignment. > This is probably sensible if the hardware can write the two bytes. > (DM might correct me there.) > Again, I don't think the FEC can do this, even if PPC does allow DMA to 4n+2 addresses for other functions. > David > > [1] The original sparc sbus 'DMA' part did multiple 16bit transfers instead > of a burst of 32bit transfers. This meant the buffer had to be misaligned > and a software copy done to align the frames. Fixed in the DMA+ part. > > [2] PCIe writes are likely to be much faster if they contain entire cache > lines of data. > ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-30 14:16 ` Eric Nelson @ 2016-10-01 19:52 ` Russell King - ARM Linux 2016-10-03 16:42 ` David Laight 2016-10-03 18:48 ` Eric Nelson 0 siblings, 2 replies; 55+ messages in thread From: Russell King - ARM Linux @ 2016-10-01 19:52 UTC (permalink / raw) To: Eric Nelson Cc: David Laight, netdev, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig On Fri, Sep 30, 2016 at 07:16:12AM -0700, Eric Nelson wrote: > On ARM, the CPU can't handle misaligned memory cycles without > taking an alignment fault and NET_IP_ALIGN is set to 2. Let's get this right... With Linux on MMU parts: On ARMv6+, unaligned memory cycles using the LDR, LDRH and corresponding store instructions are handled in hardware without any exception being raised. On pre-ARMv6, such instructions raise an alignment exception, and we fix up the load/store manually. Where things behave the same is with the LDM (load multiple) and STM (store multiple) instructions. Hardware does not fix these up if they are unaligned: it is expected that the base address will always be aligned to a 32-bit word. For some reason, the compiler guys have decided it's okay to use these instructions as an optimisation, and I see no way to disable this behaviour. > On ARM, we have CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y > but I find it hard to believe that taking alignment faults is more > efficient than adding two bytes to the start of the frame. ... see above, hence why CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set. It's only when the compiler decides to do silly things that things go wrong. However, net code does not care about that configuration setting, so it's irrelevant to this discussion. The issue with the networking layer is that it passes around structure pointers which may not be "naturally aligned" - technically it goes against the C standard specs. However, you'll find it hard to argue against this, so we have to accept that the networking people expect it to work. The optimisation that the C compiler uses (using LDM to access multiple 32-bit consecutive words) is legal and efficient when the structure pointers are aligned as it expects, but that all breaks if the pointer is not so aligned. So, raising it as a bug against the C compiler isn't going to work either. What may work is to raise a feature request with compiler people to have a mechanism to disable the LDM/STM optimisation for code where we know that pointers may not be naturally aligned. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH 3/3] net: fec: align IP header in hardware 2016-10-01 19:52 ` Russell King - ARM Linux @ 2016-10-03 16:42 ` David Laight 2016-10-03 18:48 ` Eric Nelson 1 sibling, 0 replies; 55+ messages in thread From: David Laight @ 2016-10-03 16:42 UTC (permalink / raw) To: 'Russell King - ARM Linux', Eric Nelson Cc: netdev, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig From: Russell King - ARM Linux > Sent: 01 October 2016 20:52 > On Fri, Sep 30, 2016 at 07:16:12AM -0700, Eric Nelson wrote: > > On ARM, the CPU can't handle misaligned memory cycles without > > taking an alignment fault and NET_IP_ALIGN is set to 2. > > Let's get this right... With Linux on MMU parts: > > On ARMv6+, unaligned memory cycles using the LDR, LDRH and corresponding > store instructions are handled in hardware without any exception being > raised. > > On pre-ARMv6, such instructions raise an alignment exception, and we fix > up the load/store manually. I'm not sure that is a good idea but... > Where things behave the same is with the LDM (load multiple) and STM > (store multiple) instructions. Hardware does not fix these up if they > are unaligned: it is expected that the base address will always be > aligned to a 32-bit word. > > For some reason, the compiler guys have decided it's okay to use these > instructions as an optimisation, and I see no way to disable this > behaviour. ... > The issue with the networking layer is that it passes around structure > pointers which may not be "naturally aligned" - technically it goes > against the C standard specs. However, you'll find it hard to argue > against this, so we have to accept that the networking people expect > it to work. I think it 'only' casts misaligned pointers to structure on systems where unaligned accesses are allowed. It is almost impossible to do a 'realignment copy' on (for example) sparc. > The optimisation that the C compiler uses (using LDM to access multiple > 32-bit consecutive words) is legal and efficient when the structure > pointers are aligned as it expects, but that all breaks if the pointer > is not so aligned. So, raising it as a bug against the C compiler isn't > going to work either. > > What may work is to raise a feature request with compiler people to have > a mechanism to disable the LDM/STM optimisation for code where we know > that pointers may not be naturally aligned. What happens is the relevant structures are marked 'packed'? If the compiler still generates LDM/STM that is a bug. Assuming the compiler is targeting v6+ it shouldn't generate byte accesses. David ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/3] net: fec: align IP header in hardware 2016-10-01 19:52 ` Russell King - ARM Linux 2016-10-03 16:42 ` David Laight @ 2016-10-03 18:48 ` Eric Nelson 1 sibling, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-10-03 18:48 UTC (permalink / raw) To: Russell King - ARM Linux Cc: David Laight, netdev, andrew, fugang.duan, otavio, edumazet, troy.kisky, davem, u.kleine-koenig Hi Russell, On 10/01/2016 09:52 PM, Russell King - ARM Linux wrote: > On Fri, Sep 30, 2016 at 07:16:12AM -0700, Eric Nelson wrote: >> On ARM, the CPU can't handle misaligned memory cycles without >> taking an alignment fault and NET_IP_ALIGN is set to 2. > > Let's get this right... With Linux on MMU parts: > > On ARMv6+, unaligned memory cycles using the LDR, LDRH and corresponding > store instructions are handled in hardware without any exception being > raised. > > On pre-ARMv6, such instructions raise an alignment exception, and we fix > up the load/store manually. > > Where things behave the same is with the LDM (load multiple) and STM > (store multiple) instructions. Hardware does not fix these up if they > are unaligned: it is expected that the base address will always be > aligned to a 32-bit word. > Thanks for the clarification. This helps me understand why I didn't see the exceptions that Eric warned about: https://www.spinics.net/lists/netdev/msg397012.html > For some reason, the compiler guys have decided it's okay to use these > instructions as an optimisation, and I see no way to disable this > behaviour. > >> On ARM, we have CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y >> but I find it hard to believe that taking alignment faults is more >> efficient than adding two bytes to the start of the frame. > > ... see above, hence why CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set. > It's only when the compiler decides to do silly things that things go > wrong. However, net code does not care about that configuration > setting, so it's irrelevant to this discussion. > The obfuscated optimization in ip_gro_receive doesn't help by reading two 16-bit values as a __be32: id = ntohl(*(__be32 *)&iph->id); flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)); id >>= 16; > The issue with the networking layer is that it passes around structure > pointers which may not be "naturally aligned" - technically it goes > against the C standard specs. However, you'll find it hard to argue > against this, so we have to accept that the networking people expect > it to work. > > The optimisation that the C compiler uses (using LDM to access multiple > 32-bit consecutive words) is legal and efficient when the structure > pointers are aligned as it expects, but that all breaks if the pointer > is not so aligned. So, raising it as a bug against the C compiler isn't > going to work either. > > What may work is to raise a feature request with compiler people to have > a mechanism to disable the LDM/STM optimisation for code where we know > that pointers may not be naturally aligned. > Agreed, but that's a long path even if the compiler folks agree immediately. It's probably to just fix the driver with known issues for now. Did you have any comments on the patch? I tried to pull what I found from your patch set, but only the enabling of the SHIFT16 bit. Please advise, Eric ^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH 3/3] net: fec: align IP header in hardware 2016-09-30 13:27 ` Eric Nelson 2016-09-30 13:49 ` David Laight @ 2016-10-08 2:44 ` Andy Duan 1 sibling, 0 replies; 55+ messages in thread From: Andy Duan @ 2016-10-08 2:44 UTC (permalink / raw) To: Eric Nelson, David Laight, netdev Cc: linux, andrew, otavio, edumazet, troy.kisky, davem, u.kleine-koenig From: Eric Nelson <eric@nelint.com> Sent: Friday, September 30, 2016 9:27 PM > To: David Laight <David.Laight@ACULAB.COM>; netdev@vger.kernel.org > Cc: linux@arm.linux.org.uk; andrew@lunn.ch; Andy Duan > <fugang.duan@nxp.com>; otavio@ossystems.com.br; > edumazet@google.com; troy.kisky@boundarydevices.com; > davem@davemloft.net; u.kleine-koenig@pengutronix.de > Subject: Re: [PATCH 3/3] net: fec: align IP header in hardware > > Thanks for the feedback David, > > On 09/29/2016 04:07 AM, David Laight wrote: > > From: Eric Nelson > >> Sent: 28 September 2016 18:15 > >> On 09/28/2016 09:42 AM, David Laight wrote: > >>> From: Eric Nelson > >>>> Sent: 26 September 2016 19:40 > >>>> Hi David, > >>>> > >>>> On 09/26/2016 02:26 AM, David Laight wrote: > >>>>> From: Eric Nelson > >>>>>> Sent: 24 September 2016 15:42 > >>>>>> The FEC receive accelerator (RACC) supports shifting the data > >>>>>> payload of received packets by 16-bits, which aligns the payload > >>>>>> (IP header) on a 4-byte boundary, which is, if not required, at > >>>>>> least strongly suggested by the Linux networking layer. > >>>>> ... > >>>>>> + /* align IP header */ > >>>>>> + val |= FEC_RACC_SHIFT16; > >>>>> > >>>>> I can't help feeling that there needs to be corresponding changes > >>>>> to increase the buffer size by 2 (maybe for large mtu) and to > >>>>> discard two bytes from the frame length. > >>>>> > >>>> > >>>> In the normal case, the fec driver over-allocates all receive > >>>> packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of > >>>> rx_align, which is either 0x0f (ARM) or 0x03 (PPC). > >>>> > >>>> If the frame length is less than rx_copybreak (typically 256), then > >>>> the frame length from the receive buffer descriptor is used to > >>>> control the allocation size for a copied buffer, and this will > >>>> include the two bytes of padding if RACC_SHIFT16 is set. > >>>> > >>>>> If probably ought to be predicated on NET_IP_ALIGN as well. > >>>>> > >>>> Can you elaborate? > >>> > >>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to > >>> add two bytes of 'junk' to the start of every receive frame. > >>> > >> > >> That's right. Two bytes of junk between the MAC header and the IP > >> header. > >> > >>> In the 'copybreak' case the new skb would need to be 2 bytes shorter > >>> than the length reported by the hardware, and the data copied from > >>> 2 bytes into the dma buffer. > >>> > >> > >> As it stands, the skb allocated by the copybreak routine will include > >> the two bytes of padding, and the call to skb_pull_inline will ignore > >> them. > > > > Ok, I didn't see that call being added by this patch. > > > >>> The extra 2 bytes also mean the that maximum mtu that can be > >>> received into a buffer is two bytes less. > >>> > >> > >> Right, but I think the max is already high enough that this isn't a > >> problem. > >> > >>> If someone sets the mtu to (say) 9k for jumbo frames this might matter. > >>> Even with fixed 2048 byte buffers it reduces the maximum value the > >>> mtu can be set to by 2. > >>> > >> > >> As far as I can tell, the fec driver doesn't support jumbo frames, > >> and the max frame length is currently hard-coded at PKT_MAXBUF_SIZE > (1522). > >> > >> This is well within the 2048-byte allocation, even with optional > >> headers for VLAN etc. > > > > Hmm... > > > > That (probably) means all the skb the driver allocates are actually 4k. > > It would be much better to reduce the size so that the entire skb > > (with packet buffer) is less than 2k. > > > > That seems worthwhile, but un-related to this patch. > > It appears to me that the received packets could be allocated as > > PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN > > (+2 if FEC_RACC_SHIFT16 is used) > > >>> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to > >>> start on a 4n boundary, and the skb are likely to be allocated that way. > >>> In this case you don't want to extra two bytes of 'junk'. > >>> > >> NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h > > > > Even though it is always currently set is isn't really ideal to have a > > driver that breaks if it isn't set. > > This could easily happen at some point in the future if the ethernet > > logic is put with a different cpu. > > > > After multiple reads, I'm confused about the meaning of NET_IP_ALIGN and > how it should be used. > > From Documentation/unaligned-memory-access.txt, I take it that this should > be configured on a per-architecture basis, and it seems to be set to zero on > both PPC and x86. > > I wonder if this is proper though. It seems that its' use might depend on the > I/O subsystem(s) in use as much as the architecture. > > For example, it might be desirable to have a different value for a PCIe > interface than for an integrated MAC like the FEC. > > Looking at the example of the 3c59x driver, I see a pattern of an allocation > that adds NET_IP_ALIGN followed by an skb->reserve() of NET_IP_ALIGN > before determining the target address to end up with allocation with 4n+2 > alignment. > > This seems somewhat equivalent to this patch, except that we're using the > allocated address as the target and using skb_pull_inline afterwards. > > Andy, is the FEC used on any PPC SOCs? > Sorry to reply the mail due to holiday. Currently, i.MX and ColdFire like MCF5xxx series use the driver. And ColdFire series don't define FEC_QUIRK_HAS_RACC quirk flag, so the patch don't impact ColdFire. The patch has no problem, has nothing related to DMA part. > If so, then this patch may cause a DMA of 2 extra bytes per frame > unecessarily although the driver doesn't special-case the allocation to align > the IP header, so this is still probably preferred. > > >>> OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that > >>> the data is dma'd to offset -2 in the skb and then ensure that the > >>> end of frame is set correctly. > >>> > >> > >> That's what the RACC SHIFT16 bit does. > > > > No, that causes the ethernet controller to add 2 bytes to the frame. > > You then need to change the dma target address to match. > > > > Or use skb_pull_inline to ignore the two bytes. > > > Otherwise if a new version of the silicon stops ignoring the low > > address with the frame will be misaligned in the buffer. > > > > I'm not sure I understand this. > > > The receive frame length will also (probably) be 2 larger than the > > actual frame - so you need to set the end point correctly as well. > > IP will probably ignore the 2 bytes of pad I think you are generating. > > > > The received frame length **is** 2 bytes longer, but these are eaten by > skb_pull_inline(). > > >> The FEC hardware isn't capable of DMA'ing to an un-aligned address. > >> On ARM, it requires 64-bit alignment, but suggests 128-bit alignment. > >> > >> On other (PPC?) architectures, it requires 32-bit alignment. This is > >> handled by the rx_align field. > > > > That isn't entirely relevant. > > > > If the kernel is being built with NET_IP_ALIGN set to 0 you should > > align the destination mac address on a 4n boundary (Or rather the skb > > are likely to be allocated that way). > > They're not currently allocated that way. The routine > fec_enet_alloc_rxq_buffers forces the allocations to 32 or 128-bit alignment > through the routine fec_enet_new_rxbdp(). > > > If it causes misaligned memory reads later on that is a different problem. > > That's the problem this patch is designed to address. Without this patch, the > IP header is always mis-aligned. > > > The MAC driver has aligned the frames as it was told to. > > > > David > > > > > > Regards, > > > Eric ^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH 0/3] net: fec: updates to align IP header 2016-09-24 14:42 ` [PATCH 0/3] net: fec: updates to align IP header Eric Nelson ` (2 preceding siblings ...) 2016-09-24 14:42 ` [PATCH 3/3] net: fec: align IP header in hardware Eric Nelson @ 2016-09-24 15:09 ` Andy Duan 2016-09-24 15:29 ` Eric Nelson 2016-09-27 11:40 ` David Miller 4 siblings, 1 reply; 55+ messages in thread From: Andy Duan @ 2016-09-24 15:09 UTC (permalink / raw) To: Eric Nelson, netdev Cc: linux, andrew, otavio, edumazet, troy.kisky, davem, u.kleine-koenig From: Eric Nelson <eric@nelint.com> Sent: Saturday, September 24, 2016 10:42 PM > To: netdev@vger.kernel.org > Cc: linux@arm.linux.org.uk; andrew@lunn.ch; Andy Duan > <fugang.duan@nxp.com>; otavio@ossystems.com.br; > edumazet@google.com; troy.kisky@boundarydevices.com; > davem@davemloft.net; u.kleine-koenig@pengutronix.de; Eric Nelson > <eric@nelint.com> > Subject: [PATCH 0/3] net: fec: updates to align IP header > > This patch series is the outcome of investigation into very high numbers of > alignment faults on kernel 4.1.33 from the linux-fslc > tree: > https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx > > The first two patches remove support for the receive accelerator (RACC) > from the i.MX25 and i.MX27 SoCs which don't support the function. > > The third patch enables hardware alignment of the ethernet packet payload > (and especially the IP header) to prevent alignment faults in the IP stack. > > Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed on > the order of 70k alignment faults during a 100MiB transfer using wget. > > Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed a > much more modest improvement from 10's of faults, and it's not clear why > that's the case. > > Eric Nelson (3): > net: fec: remove QUIRK_HAS_RACC from i.mx25 > net: fec: remove QUIRK_HAS_RACC from i.mx27 > net: fec: align IP header in hardware > > drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > -- > 2.7.4 I will investigate the diff between 4.1 and 4.8. Thanks. Acked-by: Fugang Duan <fugang.duan@nxp.com> ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 0/3] net: fec: updates to align IP header 2016-09-24 15:09 ` [PATCH 0/3] net: fec: updates to align IP header Andy Duan @ 2016-09-24 15:29 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-24 15:29 UTC (permalink / raw) To: Andy Duan, netdev Cc: linux, andrew, otavio, edumazet, troy.kisky, davem, u.kleine-koenig On 09/24/2016 08:09 AM, Andy Duan wrote: > From: Eric Nelson <eric@nelint.com> Sent: Saturday, September 24, 2016 10:42 PM >> To: netdev@vger.kernel.org >> Cc: linux@arm.linux.org.uk; andrew@lunn.ch; Andy Duan >> <fugang.duan@nxp.com>; otavio@ossystems.com.br; >> edumazet@google.com; troy.kisky@boundarydevices.com; >> davem@davemloft.net; u.kleine-koenig@pengutronix.de; Eric Nelson >> <eric@nelint.com> >> Subject: [PATCH 0/3] net: fec: updates to align IP header >> >> This patch series is the outcome of investigation into very high numbers of >> alignment faults on kernel 4.1.33 from the linux-fslc >> tree: >> https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx >> >> The first two patches remove support for the receive accelerator (RACC) >> from the i.MX25 and i.MX27 SoCs which don't support the function. >> >> The third patch enables hardware alignment of the ethernet packet payload >> (and especially the IP header) to prevent alignment faults in the IP stack. >> >> Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed on >> the order of 70k alignment faults during a 100MiB transfer using wget. >> >> Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed a >> much more modest improvement from 10's of faults, and it's not clear why >> that's the case. >> >> Eric Nelson (3): >> net: fec: remove QUIRK_HAS_RACC from i.mx25 >> net: fec: remove QUIRK_HAS_RACC from i.mx27 >> net: fec: align IP header in hardware >> >> drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> -- >> 2.7.4 > I will investigate the diff between 4.1 and 4.8. Thanks. > Thanks. Note that I'm not sure if the difference is 4.1 vs. 4.8 or i.MX6UL vs. i.MX6Q. > Acked-by: Fugang Duan <fugang.duan@nxp.com> > ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 0/3] net: fec: updates to align IP header 2016-09-24 14:42 ` [PATCH 0/3] net: fec: updates to align IP header Eric Nelson ` (3 preceding siblings ...) 2016-09-24 15:09 ` [PATCH 0/3] net: fec: updates to align IP header Andy Duan @ 2016-09-27 11:40 ` David Miller 4 siblings, 0 replies; 55+ messages in thread From: David Miller @ 2016-09-27 11:40 UTC (permalink / raw) To: eric Cc: netdev, linux, andrew, fugang.duan, otavio, edumazet, troy.kisky, u.kleine-koenig From: Eric Nelson <eric@nelint.com> Date: Sat, 24 Sep 2016 07:42:16 -0700 > This patch series is the outcome of investigation into very high > numbers of alignment faults on kernel 4.1.33 from the linux-fslc > tree: > https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx > > The first two patches remove support for the receive accelerator (RACC) from > the i.MX25 and i.MX27 SoCs which don't support the function. > > The third patch enables hardware alignment of the ethernet packet payload > (and especially the IP header) to prevent alignment faults in the IP stack. > > Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed > on the order of 70k alignment faults during a 100MiB transfer using > wget. > > Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed > a much more modest improvement from 10's of faults, and it's not clear > why that's the case. Series applied and queued up for -stable. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 17:33 ` Eric Nelson @ 2016-09-24 2:43 ` David Miller -1 siblings, 0 replies; 55+ messages in thread From: David Miller @ 2016-09-24 2:43 UTC (permalink / raw) To: eric Cc: edumazet, linux-arm-kernel, netdev, rmk+kernel, fugang.duan, troy.kisky, otavio, cjb.sw.nospam From: Eric Nelson <eric@nelint.com> Date: Fri, 23 Sep 2016 10:33:29 -0700 > Since the hardware requires longword alignment for its' DMA transfers, > aligning the IP header will require a memcpy, right? I wish hardware designers didn't do this. There is no conflict between DMA alignment and properly offseting the packet data by two bytes. All hardware designers have to do is allow 2 padding bytes to be emitted by the chip before the actual packet data. Then the longword or whatever DMA transfer alignment is met whilst still giving the necessary flexibility for where the packet data lands. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-24 2:43 ` David Miller 0 siblings, 0 replies; 55+ messages in thread From: David Miller @ 2016-09-24 2:43 UTC (permalink / raw) To: linux-arm-kernel From: Eric Nelson <eric@nelint.com> Date: Fri, 23 Sep 2016 10:33:29 -0700 > Since the hardware requires longword alignment for its' DMA transfers, > aligning the IP header will require a memcpy, right? I wish hardware designers didn't do this. There is no conflict between DMA alignment and properly offseting the packet data by two bytes. All hardware designers have to do is allow 2 padding bytes to be emitted by the chip before the actual packet data. Then the longword or whatever DMA transfer alignment is met whilst still giving the necessary flexibility for where the packet data lands. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-24 2:43 ` David Miller @ 2016-09-24 12:27 ` Eric Nelson -1 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-24 12:27 UTC (permalink / raw) To: David Miller Cc: edumazet, linux-arm-kernel, netdev, rmk+kernel, fugang.duan, troy.kisky, otavio, cjb.sw.nospam Hi David, On 09/23/2016 07:43 PM, David Miller wrote: > From: Eric Nelson <eric@nelint.com> > Date: Fri, 23 Sep 2016 10:33:29 -0700 > >> Since the hardware requires longword alignment for its' DMA transfers, >> aligning the IP header will require a memcpy, right? > > I wish hardware designers didn't do this. > > There is no conflict between DMA alignment and properly offseting > the packet data by two bytes. > > All hardware designers have to do is allow 2 padding bytes to be > emitted by the chip before the actual packet data. > Andrew Lunn pointed out that the hardware does support this, and I just pushed a patch for the vendor kernel to the meta-freescale mailing list: https://lists.yoctoproject.org/pipermail/meta-freescale/2016-September/019228.html > Then the longword or whatever DMA transfer alignment is met > whilst still giving the necessary flexibility for where the > packet data lands. > Right. A relatively small change fixes things right up. Many thanks to Andrew for pointing this out and Russell for providing the basis for my patch. I'll re-work this for the up-stream kernel when I get out from under a couple of unrelated things. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-24 12:27 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-24 12:27 UTC (permalink / raw) To: linux-arm-kernel Hi David, On 09/23/2016 07:43 PM, David Miller wrote: > From: Eric Nelson <eric@nelint.com> > Date: Fri, 23 Sep 2016 10:33:29 -0700 > >> Since the hardware requires longword alignment for its' DMA transfers, >> aligning the IP header will require a memcpy, right? > > I wish hardware designers didn't do this. > > There is no conflict between DMA alignment and properly offseting > the packet data by two bytes. > > All hardware designers have to do is allow 2 padding bytes to be > emitted by the chip before the actual packet data. > Andrew Lunn pointed out that the hardware does support this, and I just pushed a patch for the vendor kernel to the meta-freescale mailing list: https://lists.yoctoproject.org/pipermail/meta-freescale/2016-September/019228.html > Then the longword or whatever DMA transfer alignment is met > whilst still giving the necessary flexibility for where the > packet data lands. > Right. A relatively small change fixes things right up. Many thanks to Andrew for pointing this out and Russell for providing the basis for my patch. I'll re-work this for the up-stream kernel when I get out from under a couple of unrelated things. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 17:19 ` Eric Nelson @ 2016-09-23 17:37 ` Russell King - ARM Linux -1 siblings, 0 replies; 55+ messages in thread From: Russell King - ARM Linux @ 2016-09-23 17:37 UTC (permalink / raw) To: Eric Nelson Cc: Eric Dumazet, linux-arm-kernel, netdev, Fugang Duan, Troy Kisky, Otavio Salvador, Simone On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote: > Oddly, it does prevent the vast majority (90%+) of the alignment errors. > > I believe this is because the compiler is generating an ldm instruction > when the ntohl() call is used, but I'm stumped about why these aren't > generating faults: ldm generates alignment faults when the address is not aligned to a 32-bit boundary. ldr on ARMv6+ does not. > I don't think that's the case. > > # CONFIG_IPV6_GRE is not set > > Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on > a 4-byte boundary. > > Does the ldm instruction require 8-byte alignment? > > There's definitely a compiler-version dependency involved here, > since using gcc 4.9 also reduced the number of faults dramatically. Well, I don't think it's that gcc related: User: 0 System: 312855 (ip6_route_input+0x6c/0x1e0) Skipped: 0 Half: 0 Word: 0 DWord: 2 Multi: 312853 c06d8998 <ip6_route_input>: c06d89ac: e1a04000 mov r4, r0 c06d89b0: e1d489b4 ldrh r8, [r4, #148] ; 0x94 c06d89b8: e594a0a0 ldr sl, [r4, #160] ; 0xa0 c06d89cc: e08ac008 add ip, sl, r8 c06d89d4: e28c3018 add r3, ip, #24 c06d89dc: e28c7008 add r7, ip, #8 c06d89e4: e893000f ldm r3, {r0, r1, r2, r3} c06d89ec: e24be044 sub lr, fp, #68 ; 0x44 c06d89f4: e24b5054 sub r5, fp, #84 ; 0x54 c06d89fc: e885000f stm r5, {r0, r1, r2, r3} c06d8a04: e897000f ldm r7, {r0, r1, r2, r3} c06d8a10: e88e000f stm lr, {r0, r1, r2, r3} This is from: struct flowi6 fl6 = { .flowi6_iif = l3mdev_fib_oif(skb->dev), .daddr = iph->daddr, .saddr = iph->saddr, .flowlabel = ip6_flowinfo(iph), .flowi6_mark = skb->mark, .flowi6_proto = iph->nexthdr, }; specifically, I suspect, the saddr and daddr initialisations. There's not much to get away from this - the FEC on iMX requires a 16-byte alignment for DMA addresses, which violates the network stack's requirement for the ethernet packet to be received with a two byte offset. So the IP header (and IPv6 headers) will always be mis-aligned in memory, which leads to a huge number of alignment faults. There's not much getting away from this - the problem is not in the networking stack, but the FEC hardware/network driver. See: struct fec_enet_private *fep = netdev_priv(ndev); int off; off = ((unsigned long)skb->data) & fep->rx_align; if (off) skb_reserve(skb, fep->rx_align + 1 - off); bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE)); in fec_enet_new_rxbdp(). -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 17:37 ` Russell King - ARM Linux 0 siblings, 0 replies; 55+ messages in thread From: Russell King - ARM Linux @ 2016-09-23 17:37 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote: > Oddly, it does prevent the vast majority (90%+) of the alignment errors. > > I believe this is because the compiler is generating an ldm instruction > when the ntohl() call is used, but I'm stumped about why these aren't > generating faults: ldm generates alignment faults when the address is not aligned to a 32-bit boundary. ldr on ARMv6+ does not. > I don't think that's the case. > > # CONFIG_IPV6_GRE is not set > > Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on > a 4-byte boundary. > > Does the ldm instruction require 8-byte alignment? > > There's definitely a compiler-version dependency involved here, > since using gcc 4.9 also reduced the number of faults dramatically. Well, I don't think it's that gcc related: User: 0 System: 312855 (ip6_route_input+0x6c/0x1e0) Skipped: 0 Half: 0 Word: 0 DWord: 2 Multi: 312853 c06d8998 <ip6_route_input>: c06d89ac: e1a04000 mov r4, r0 c06d89b0: e1d489b4 ldrh r8, [r4, #148] ; 0x94 c06d89b8: e594a0a0 ldr sl, [r4, #160] ; 0xa0 c06d89cc: e08ac008 add ip, sl, r8 c06d89d4: e28c3018 add r3, ip, #24 c06d89dc: e28c7008 add r7, ip, #8 c06d89e4: e893000f ldm r3, {r0, r1, r2, r3} c06d89ec: e24be044 sub lr, fp, #68 ; 0x44 c06d89f4: e24b5054 sub r5, fp, #84 ; 0x54 c06d89fc: e885000f stm r5, {r0, r1, r2, r3} c06d8a04: e897000f ldm r7, {r0, r1, r2, r3} c06d8a10: e88e000f stm lr, {r0, r1, r2, r3} This is from: struct flowi6 fl6 = { .flowi6_iif = l3mdev_fib_oif(skb->dev), .daddr = iph->daddr, .saddr = iph->saddr, .flowlabel = ip6_flowinfo(iph), .flowi6_mark = skb->mark, .flowi6_proto = iph->nexthdr, }; specifically, I suspect, the saddr and daddr initialisations. There's not much to get away from this - the FEC on iMX requires a 16-byte alignment for DMA addresses, which violates the network stack's requirement for the ethernet packet to be received with a two byte offset. So the IP header (and IPv6 headers) will always be mis-aligned in memory, which leads to a huge number of alignment faults. There's not much getting away from this - the problem is not in the networking stack, but the FEC hardware/network driver. See: struct fec_enet_private *fep = netdev_priv(ndev); int off; off = ((unsigned long)skb->data) & fep->rx_align; if (off) skb_reserve(skb, fep->rx_align + 1 - off); bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE)); in fec_enet_new_rxbdp(). -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 17:37 ` Russell King - ARM Linux @ 2016-09-23 18:26 ` Eric Nelson -1 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 18:26 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Eric Dumazet, linux-arm-kernel, netdev, Fugang Duan, Troy Kisky, Otavio Salvador, Simone Thanks Russell, On 09/23/2016 10:37 AM, Russell King - ARM Linux wrote: > On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote: >> Oddly, it does prevent the vast majority (90%+) of the alignment errors. >> >> I believe this is because the compiler is generating an ldm instruction >> when the ntohl() call is used, but I'm stumped about why these aren't >> generating faults: After looking at it, I have to think that the code that reads iph->id is just hit more frequently than the other code in this routine. > > ldm generates alignment faults when the address is not aligned to a > 32-bit boundary. ldr on ARMv6+ does not. > >> I don't think that's the case. >> >> # CONFIG_IPV6_GRE is not set >> >> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on >> a 4-byte boundary. >> >> Does the ldm instruction require 8-byte alignment? >> >> There's definitely a compiler-version dependency involved here, >> since using gcc 4.9 also reduced the number of faults dramatically. > > Well, I don't think it's that gcc related: > I can only say that I noticed a dramatic drop in the number of faults, and didn't see the inet_gro_receive reported in /proc/cpu/alignment with gcc 4.9 when trying to identify the issue. > User: 0 > System: 312855 (ip6_route_input+0x6c/0x1e0) > Skipped: 0 > Half: 0 > Word: 0 > DWord: 2 > Multi: 312853 > > c06d8998 <ip6_route_input>: > c06d89ac: e1a04000 mov r4, r0 > c06d89b0: e1d489b4 ldrh r8, [r4, #148] ; 0x94 > c06d89b8: e594a0a0 ldr sl, [r4, #160] ; 0xa0 > c06d89cc: e08ac008 add ip, sl, r8 > c06d89d4: e28c3018 add r3, ip, #24 > c06d89dc: e28c7008 add r7, ip, #8 > c06d89e4: e893000f ldm r3, {r0, r1, r2, r3} > c06d89ec: e24be044 sub lr, fp, #68 ; 0x44 > c06d89f4: e24b5054 sub r5, fp, #84 ; 0x54 > c06d89fc: e885000f stm r5, {r0, r1, r2, r3} > c06d8a04: e897000f ldm r7, {r0, r1, r2, r3} > c06d8a10: e88e000f stm lr, {r0, r1, r2, r3} > > This is from: > > struct flowi6 fl6 = { > .flowi6_iif = l3mdev_fib_oif(skb->dev), > .daddr = iph->daddr, > .saddr = iph->saddr, > .flowlabel = ip6_flowinfo(iph), > .flowi6_mark = skb->mark, > .flowi6_proto = iph->nexthdr, > }; > > specifically, I suspect, the saddr and daddr initialisations. > > There's not much to get away from this - the FEC on iMX requires a > 16-byte alignment for DMA addresses, which violates the network > stack's requirement for the ethernet packet to be received with a > two byte offset. So the IP header (and IPv6 headers) will always > be mis-aligned in memory, which leads to a huge number of alignment > faults. > > There's not much getting away from this - the problem is not in the > networking stack, but the FEC hardware/network driver. See: > > struct fec_enet_private *fep = netdev_priv(ndev); > int off; > > off = ((unsigned long)skb->data) & fep->rx_align; > if (off) > skb_reserve(skb, fep->rx_align + 1 - off); > > bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE)); > > in fec_enet_new_rxbdp(). > So the question is: should we just live with this and acknowledge a performance penalty of bad alignment or do something about it? I'm not sure the cost (or the details) of Eric's proposed fix of allocating and copying the header to another skb. The original report was of bad network performance, but I haven't been able to see an impact doing some simple tests using wget and SSH. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 18:26 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 18:26 UTC (permalink / raw) To: linux-arm-kernel Thanks Russell, On 09/23/2016 10:37 AM, Russell King - ARM Linux wrote: > On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote: >> Oddly, it does prevent the vast majority (90%+) of the alignment errors. >> >> I believe this is because the compiler is generating an ldm instruction >> when the ntohl() call is used, but I'm stumped about why these aren't >> generating faults: After looking at it, I have to think that the code that reads iph->id is just hit more frequently than the other code in this routine. > > ldm generates alignment faults when the address is not aligned to a > 32-bit boundary. ldr on ARMv6+ does not. > >> I don't think that's the case. >> >> # CONFIG_IPV6_GRE is not set >> >> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on >> a 4-byte boundary. >> >> Does the ldm instruction require 8-byte alignment? >> >> There's definitely a compiler-version dependency involved here, >> since using gcc 4.9 also reduced the number of faults dramatically. > > Well, I don't think it's that gcc related: > I can only say that I noticed a dramatic drop in the number of faults, and didn't see the inet_gro_receive reported in /proc/cpu/alignment with gcc 4.9 when trying to identify the issue. > User: 0 > System: 312855 (ip6_route_input+0x6c/0x1e0) > Skipped: 0 > Half: 0 > Word: 0 > DWord: 2 > Multi: 312853 > > c06d8998 <ip6_route_input>: > c06d89ac: e1a04000 mov r4, r0 > c06d89b0: e1d489b4 ldrh r8, [r4, #148] ; 0x94 > c06d89b8: e594a0a0 ldr sl, [r4, #160] ; 0xa0 > c06d89cc: e08ac008 add ip, sl, r8 > c06d89d4: e28c3018 add r3, ip, #24 > c06d89dc: e28c7008 add r7, ip, #8 > c06d89e4: e893000f ldm r3, {r0, r1, r2, r3} > c06d89ec: e24be044 sub lr, fp, #68 ; 0x44 > c06d89f4: e24b5054 sub r5, fp, #84 ; 0x54 > c06d89fc: e885000f stm r5, {r0, r1, r2, r3} > c06d8a04: e897000f ldm r7, {r0, r1, r2, r3} > c06d8a10: e88e000f stm lr, {r0, r1, r2, r3} > > This is from: > > struct flowi6 fl6 = { > .flowi6_iif = l3mdev_fib_oif(skb->dev), > .daddr = iph->daddr, > .saddr = iph->saddr, > .flowlabel = ip6_flowinfo(iph), > .flowi6_mark = skb->mark, > .flowi6_proto = iph->nexthdr, > }; > > specifically, I suspect, the saddr and daddr initialisations. > > There's not much to get away from this - the FEC on iMX requires a > 16-byte alignment for DMA addresses, which violates the network > stack's requirement for the ethernet packet to be received with a > two byte offset. So the IP header (and IPv6 headers) will always > be mis-aligned in memory, which leads to a huge number of alignment > faults. > > There's not much getting away from this - the problem is not in the > networking stack, but the FEC hardware/network driver. See: > > struct fec_enet_private *fep = netdev_priv(ndev); > int off; > > off = ((unsigned long)skb->data) & fep->rx_align; > if (off) > skb_reserve(skb, fep->rx_align + 1 - off); > > bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE)); > > in fec_enet_new_rxbdp(). > So the question is: should we just live with this and acknowledge a performance penalty of bad alignment or do something about it? I'm not sure the cost (or the details) of Eric's proposed fix of allocating and copying the header to another skb. The original report was of bad network performance, but I haven't been able to see an impact doing some simple tests using wget and SSH. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 18:26 ` Eric Nelson @ 2016-09-23 18:37 ` Russell King - ARM Linux -1 siblings, 0 replies; 55+ messages in thread From: Russell King - ARM Linux @ 2016-09-23 18:37 UTC (permalink / raw) To: Eric Nelson Cc: Eric Dumazet, linux-arm-kernel, netdev, Fugang Duan, Troy Kisky, Otavio Salvador, Simone On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote: > So the question is: should we just live with this and acknowledge a > performance penalty of bad alignment or do something about it? Well, I've no interest in trying to do anything with the FEC driver anymore, as I'll just generate another big patch stack which won't make it into the kernel in a timely fashion - my last attempt at improving the FEC driver was dogged with conflicting changes and I gave up with it in the end. I ended up spending a full cycle rebasing, re-testing, and re-evaluating their performance only to find that I'd missed the merge window again, and other conflicting changes got merged which meant that I had to start from the beginning again. > I'm not sure the cost (or the details) of Eric's proposed fix of allocating > and copying the header to another skb. I had a quick look at this, and although Eric's idea may be a good idea, it doesn't contain enough details for me to be able to implement it - eg, I've no idea how to attach the 128-byte skb to the beginning of a previously allocated skb containing the rest of the packet. I've just looked through linux/skbuff.h and I can't see anything that takes two sk_buff's that would do the job. However, I don't think that's necessary in this case, because the iMX6 FEC supports the 16-bit alignment of the packet, if only it was enabled in hardware and the driver caters for it. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 18:37 ` Russell King - ARM Linux 0 siblings, 0 replies; 55+ messages in thread From: Russell King - ARM Linux @ 2016-09-23 18:37 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote: > So the question is: should we just live with this and acknowledge a > performance penalty of bad alignment or do something about it? Well, I've no interest in trying to do anything with the FEC driver anymore, as I'll just generate another big patch stack which won't make it into the kernel in a timely fashion - my last attempt at improving the FEC driver was dogged with conflicting changes and I gave up with it in the end. I ended up spending a full cycle rebasing, re-testing, and re-evaluating their performance only to find that I'd missed the merge window again, and other conflicting changes got merged which meant that I had to start from the beginning again. > I'm not sure the cost (or the details) of Eric's proposed fix of allocating > and copying the header to another skb. I had a quick look at this, and although Eric's idea may be a good idea, it doesn't contain enough details for me to be able to implement it - eg, I've no idea how to attach the 128-byte skb to the beginning of a previously allocated skb containing the rest of the packet. I've just looked through linux/skbuff.h and I can't see anything that takes two sk_buff's that would do the job. However, I don't think that's necessary in this case, because the iMX6 FEC supports the 16-bit alignment of the packet, if only it was enabled in hardware and the driver caters for it. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 18:37 ` Russell King - ARM Linux @ 2016-09-23 18:49 ` Eric Nelson -1 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 18:49 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Eric Dumazet, linux-arm-kernel, netdev, Fugang Duan, Troy Kisky, Otavio Salvador, Simone Thanks Russell, On 09/23/2016 11:37 AM, Russell King - ARM Linux wrote: > On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote: >> So the question is: should we just live with this and acknowledge a >> performance penalty of bad alignment or do something about it? > > Well, I've no interest in trying to do anything with the FEC driver > anymore, as I'll just generate another big patch stack which won't > make it into the kernel in a timely fashion - my last attempt at > improving the FEC driver was dogged with conflicting changes and I > gave up with it in the end. I ended up spending a full cycle > rebasing, re-testing, and re-evaluating their performance only to find > that I'd missed the merge window again, and other conflicting changes > got merged which meant that I had to start from the beginning again. > That's sad. I recall reading your notes on that patch series and it was a model for how to structure and document a patch set. I hadn't noticed that you abandoned it and it's frustrating that the merge process prevented your efforts from being used. I'm also disheartened to hear your frustration about getting things pushed up-stream and the entire Linux community should take note. >> I'm not sure the cost (or the details) of Eric's proposed fix of allocating >> and copying the header to another skb. > > I had a quick look at this, and although Eric's idea may be a good > idea, it doesn't contain enough details for me to be able to > implement it - eg, I've no idea how to attach the 128-byte skb to the > beginning of a previously allocated skb containing the rest of the > packet. I've just looked through linux/skbuff.h and I can't see > anything that takes two sk_buff's that would do the job. > > However, I don't think that's necessary in this case, because the > iMX6 FEC supports the 16-bit alignment of the packet, if only it was > enabled in hardware and the driver caters for it. > Right. If the hardware supports placing things at a suitable address, that's the right approach. I'll try to review your earlier patch set and at least find a way to address the alignment issues. I'm a bit booked until LinuxCon but will try to get something out soon. Regards, Eric ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 18:49 ` Eric Nelson 0 siblings, 0 replies; 55+ messages in thread From: Eric Nelson @ 2016-09-23 18:49 UTC (permalink / raw) To: linux-arm-kernel Thanks Russell, On 09/23/2016 11:37 AM, Russell King - ARM Linux wrote: > On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote: >> So the question is: should we just live with this and acknowledge a >> performance penalty of bad alignment or do something about it? > > Well, I've no interest in trying to do anything with the FEC driver > anymore, as I'll just generate another big patch stack which won't > make it into the kernel in a timely fashion - my last attempt at > improving the FEC driver was dogged with conflicting changes and I > gave up with it in the end. I ended up spending a full cycle > rebasing, re-testing, and re-evaluating their performance only to find > that I'd missed the merge window again, and other conflicting changes > got merged which meant that I had to start from the beginning again. > That's sad. I recall reading your notes on that patch series and it was a model for how to structure and document a patch set. I hadn't noticed that you abandoned it and it's frustrating that the merge process prevented your efforts from being used. I'm also disheartened to hear your frustration about getting things pushed up-stream and the entire Linux community should take note. >> I'm not sure the cost (or the details) of Eric's proposed fix of allocating >> and copying the header to another skb. > > I had a quick look at this, and although Eric's idea may be a good > idea, it doesn't contain enough details for me to be able to > implement it - eg, I've no idea how to attach the 128-byte skb to the > beginning of a previously allocated skb containing the rest of the > packet. I've just looked through linux/skbuff.h and I can't see > anything that takes two sk_buff's that would do the job. > > However, I don't think that's necessary in this case, because the > iMX6 FEC supports the 16-bit alignment of the packet, if only it was > enabled in hardware and the driver caters for it. > Right. If the hardware supports placing things at a suitable address, that's the right approach. I'll try to review your earlier patch set and at least find a way to address the alignment issues. I'm a bit booked until LinuxCon but will try to get something out soon. Regards, Eric ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Alignment issues with freescale FEC driver 2016-09-23 18:37 ` Russell King - ARM Linux @ 2016-09-23 20:22 ` Uwe Kleine-König -1 siblings, 0 replies; 55+ messages in thread From: Uwe Kleine-König @ 2016-09-23 20:22 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Eric Nelson, Fugang Duan, Otavio Salvador, netdev, Troy Kisky, Eric Dumazet, Simone, linux-arm-kernel, kernel Hello Russell, On Fri, Sep 23, 2016 at 07:37:25PM +0100, Russell King - ARM Linux wrote: > On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote: > > So the question is: should we just live with this and acknowledge a > > performance penalty of bad alignment or do something about it? > > Well, I've no interest in trying to do anything with the FEC driver > anymore, as I'll just generate another big patch stack which won't > make it into the kernel in a timely fashion - my last attempt at > improving the FEC driver was dogged with conflicting changes and I > gave up with it in the end. I ended up spending a full cycle > rebasing, re-testing, and re-evaluating their performance only to find > that I'd missed the merge window again, and other conflicting changes > got merged which meant that I had to start from the beginning again. I'm not included in the set of people who are responsible to review and merge fec patches, but I'd be surprised if you couldn't get an exclusive lock for that driver. Something like: After 4.X-rc1 the fec isn't touched any more until you got your series ready for the 4.X+1 merge window. Of course some fixes might have to go in, but these hopefully won't disturb much. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 55+ messages in thread
* Alignment issues with freescale FEC driver @ 2016-09-23 20:22 ` Uwe Kleine-König 0 siblings, 0 replies; 55+ messages in thread From: Uwe Kleine-König @ 2016-09-23 20:22 UTC (permalink / raw) To: linux-arm-kernel Hello Russell, On Fri, Sep 23, 2016 at 07:37:25PM +0100, Russell King - ARM Linux wrote: > On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote: > > So the question is: should we just live with this and acknowledge a > > performance penalty of bad alignment or do something about it? > > Well, I've no interest in trying to do anything with the FEC driver > anymore, as I'll just generate another big patch stack which won't > make it into the kernel in a timely fashion - my last attempt at > improving the FEC driver was dogged with conflicting changes and I > gave up with it in the end. I ended up spending a full cycle > rebasing, re-testing, and re-evaluating their performance only to find > that I'd missed the merge window again, and other conflicting changes > got merged which meant that I had to start from the beginning again. I'm not included in the set of people who are responsible to review and merge fec patches, but I'd be surprised if you couldn't get an exclusive lock for that driver. Something like: After 4.X-rc1 the fec isn't touched any more until you got your series ready for the 4.X+1 merge window. Of course some fixes might have to go in, but these hopefully won't disturb much. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 55+ messages in thread
end of thread, other threads:[~2016-10-08 7:18 UTC | newest] Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-23 16:43 Alignment issues with freescale FEC driver Eric Nelson 2016-09-23 16:43 ` Eric Nelson 2016-09-23 16:54 ` Eric Dumazet 2016-09-23 16:54 ` Eric Dumazet 2016-09-23 17:19 ` Eric Nelson 2016-09-23 17:19 ` Eric Nelson 2016-09-23 17:33 ` Eric Nelson 2016-09-23 17:33 ` Eric Nelson 2016-09-23 18:13 ` Andrew Lunn 2016-09-23 18:13 ` Andrew Lunn 2016-09-23 18:30 ` Russell King - ARM Linux 2016-09-23 18:30 ` Russell King - ARM Linux 2016-09-23 18:39 ` Eric Nelson 2016-09-23 18:39 ` Eric Nelson 2016-09-23 18:35 ` Eric Nelson 2016-09-23 18:35 ` Eric Nelson 2016-09-24 2:45 ` David Miller 2016-09-24 2:45 ` David Miller 2016-09-24 5:13 ` Andy Duan 2016-09-24 5:13 ` Andy Duan 2016-09-24 14:42 ` [PATCH 0/3] net: fec: updates to align IP header Eric Nelson 2016-09-24 14:42 ` [PATCH 1/3] net: fec: remove QUIRK_HAS_RACC from i.mx25 Eric Nelson 2016-09-24 14:42 ` [PATCH 2/3] net: fec: remove QUIRK_HAS_RACC from i.mx27 Eric Nelson 2016-09-24 14:42 ` [PATCH 3/3] net: fec: align IP header in hardware Eric Nelson 2016-09-26 9:26 ` David Laight 2016-09-26 18:39 ` Eric Nelson 2016-09-28 16:42 ` David Laight 2016-09-28 17:14 ` Eric Nelson 2016-09-28 17:25 ` Russell King - ARM Linux 2016-09-28 18:01 ` Eric Nelson 2016-09-29 11:07 ` David Laight 2016-09-30 13:27 ` Eric Nelson 2016-09-30 13:49 ` David Laight 2016-09-30 14:16 ` Eric Nelson 2016-10-01 19:52 ` Russell King - ARM Linux 2016-10-03 16:42 ` David Laight 2016-10-03 18:48 ` Eric Nelson 2016-10-08 2:44 ` Andy Duan 2016-09-24 15:09 ` [PATCH 0/3] net: fec: updates to align IP header Andy Duan 2016-09-24 15:29 ` Eric Nelson 2016-09-27 11:40 ` David Miller 2016-09-24 2:43 ` Alignment issues with freescale FEC driver David Miller 2016-09-24 2:43 ` David Miller 2016-09-24 12:27 ` Eric Nelson 2016-09-24 12:27 ` Eric Nelson 2016-09-23 17:37 ` Russell King - ARM Linux 2016-09-23 17:37 ` Russell King - ARM Linux 2016-09-23 18:26 ` Eric Nelson 2016-09-23 18:26 ` Eric Nelson 2016-09-23 18:37 ` Russell King - ARM Linux 2016-09-23 18:37 ` Russell King - ARM Linux 2016-09-23 18:49 ` Eric Nelson 2016-09-23 18:49 ` Eric Nelson 2016-09-23 20:22 ` Uwe Kleine-König 2016-09-23 20:22 ` Uwe Kleine-König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.