Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion
From: Florian Fainelli Date: Wed, 29 Jul 2015 15:32:37 -0700 > This patch series adds two new feature flags to allow Ethernet controllers > capable of doing Ethernet switch proprietary tag insertion/extraction to > benefit from that. > > The last two patches modify the Broadcom tag parsing code in DSA to utilize > that feature and finally, the SYSTEMPORT controller driver is updated to > turn on that feature when requested. > > The ethtool patch will follow-up shortly as well. > > Insertion on transmit is still work in progress and requires more turning on > how the SYSTEMPORT transmit queues are mapped to their respective switch ports > and queues (there are 32 queues on SYSTEMPORT to output to a 4-port switch > each > port with 8 transmit queues). This looks fine, series applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion
From: David Miller Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT) > This looks fine, series applied, thanks. I think your control block is too large, you'll need to rework this somehow. In function ‘dsa_copy_brcm_tag’, inlined from ‘bcm_sysport_desc_rx’ at drivers/net/ethernet/broadcom/bcmsysport.c:707:4, inlined from ‘bcm_sysport_poll’ at drivers/net/ethernet/broadcom/bcmsysport.c:864:12: include/linux/compiler.h:447:38: error: call to ‘__compiletime_assert_2016’ declared with attribute error: BUILD_BUG_ON failed: sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) ^ include/linux/compiler.h:430:4: note: in definition of macro ‘__compiletime_assert’ prefix ## suffix();\ ^ include/linux/compiler.h:447:2: note: in expansion of macro ‘_compiletime_assert’ _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) ^ include/linux/bug.h:50:37: note: in expansion of macro ‘compiletime_assert’ #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) ^ include/linux/bug.h:74:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) ^ include/linux/netdevice.h:2016:2: note: in expansion of macro ‘BUILD_BUG_ON’ BUILD_BUG_ON(sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN); ^ scripts/Makefile.build:264: recipe for target 'drivers/net/ethernet/broadcom/bcmsysport.o' failed
Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion
On 30/07/15 15:51, David Miller wrote: > From: David Miller > Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT) > >> This looks fine, series applied, thanks. > > I think your control block is too large, you'll need to rework this > somehow. Interesting, this only seems to show up with 64-bits build, which is why I did not catch this earlier (building for ARMv7 typically), sorry about that, I will come up with a fix. > > In function ‘dsa_copy_brcm_tag’, > inlined from ‘bcm_sysport_desc_rx’ at > drivers/net/ethernet/broadcom/bcmsysport.c:707:4, > inlined from ‘bcm_sysport_poll’ at > drivers/net/ethernet/broadcom/bcmsysport.c:864:12: > include/linux/compiler.h:447:38: error: call to ‘__compiletime_assert_2016’ > declared with attribute error: BUILD_BUG_ON failed: sizeof(skb->cb) - > sizeof(struct napi_gro_cb) < BRCM_TAG_LEN > _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) > ^ > include/linux/compiler.h:430:4: note: in definition of macro > ‘__compiletime_assert’ > prefix ## suffix();\ > ^ > include/linux/compiler.h:447:2: note: in expansion of macro > ‘_compiletime_assert’ > _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) > ^ > include/linux/bug.h:50:37: note: in expansion of macro ‘compiletime_assert’ > #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > ^ > include/linux/bug.h:74:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ > BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) > ^ > include/linux/netdevice.h:2016:2: note: in expansion of macro ‘BUILD_BUG_ON’ > BUILD_BUG_ON(sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN); > ^ > scripts/Makefile.build:264: recipe for target > 'drivers/net/ethernet/broadcom/bcmsysport.o' failed > -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion
On 30/07/15 15:51, David Miller wrote: > From: David Miller > Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT) > >> This looks fine, series applied, thanks. > > I think your control block is too large, you'll need to rework this > somehow. So napi_gro_cb really is 48 bytes on 64-bits architectures (had not realized it was so big). What would you recommend to do here considering that this driver is currently used on 32-bits platforms, but I see no reason why someone would no want to use this feature on a 64-bit platform, yet we are competing with napi_gro_cb, and adding a new skbuff member is pretty much a no-no? Would it be acceptable to have a new member which is ifdef CONFIG_NET_DSA_TAG_BRCM? FWIW, this does provide a small 2-3% throughput increase for RX. > > In function ‘dsa_copy_brcm_tag’, > inlined from ‘bcm_sysport_desc_rx’ at > drivers/net/ethernet/broadcom/bcmsysport.c:707:4, > inlined from ‘bcm_sysport_poll’ at > drivers/net/ethernet/broadcom/bcmsysport.c:864:12: > include/linux/compiler.h:447:38: error: call to ‘__compiletime_assert_2016’ > declared with attribute error: BUILD_BUG_ON failed: sizeof(skb->cb) - > sizeof(struct napi_gro_cb) < BRCM_TAG_LEN > _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) > ^ > include/linux/compiler.h:430:4: note: in definition of macro > ‘__compiletime_assert’ > prefix ## suffix();\ > ^ > include/linux/compiler.h:447:2: note: in expansion of macro > ‘_compiletime_assert’ > _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) > ^ > include/linux/bug.h:50:37: note: in expansion of macro ‘compiletime_assert’ > #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > ^ > include/linux/bug.h:74:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ > BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) > ^ > include/linux/netdevice.h:2016:2: note: in expansion of macro ‘BUILD_BUG_ON’ > BUILD_BUG_ON(sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN); > ^ > scripts/Makefile.build:264: recipe for target > 'drivers/net/ethernet/broadcom/bcmsysport.o' failed > -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion
Hi Florian, On 07/31/2015 01:51 PM, Florian Fainelli wrote: > On 30/07/15 15:51, David Miller wrote: >> From: David Miller >> Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT) >> >>> This looks fine, series applied, thanks. >> >> I think your control block is too large, you'll need to rework this >> somehow. > > So napi_gro_cb really is 48 bytes on 64-bits architectures (had not > realized it was so big). > > What would you recommend to do here considering that this driver is > currently used on 32-bits platforms, but I see no reason why someone > would no want to use this feature on a 64-bit platform, I haven't read a lot of the detail in your patch series yet nor do I understand the ramifications of the error this has triggered. But I will stick my hand up for this point. We do have a router with a 64-bit CPU and a bcm L2 switch (b53 is the code name being flung around). None of this is upstream (yet) but the CPU vendor has at least come to the party and has landed patches in 4.2. We'd probably have to do the switch support ourselves (perhaps based on code from openWRT). > yet we are > competing with napi_gro_cb, and adding a new skbuff member is pretty > much a no-no? Would it be acceptable to have a new member which is ifdef > CONFIG_NET_DSA_TAG_BRCM? > > FWIW, this does provide a small 2-3% throughput increase for RX. I'll leaves this for the netdev folks to comment on. I just wanted to let you know that such a use-case _may_ exist. >> >> In function ‘dsa_copy_brcm_tag’, >> inlined from ‘bcm_sysport_desc_rx’ at >> drivers/net/ethernet/broadcom/bcmsysport.c:707:4, >> inlined from ‘bcm_sysport_poll’ at >> drivers/net/ethernet/broadcom/bcmsysport.c:864:12: >> include/linux/compiler.h:447:38: error: call to ‘__compiletime_assert_2016’ >> declared with attribute error: BUILD_BUG_ON failed: sizeof(skb->cb) - >> sizeof(struct napi_gro_cb) < BRCM_TAG_LEN >>_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) >>^ >> include/linux/compiler.h:430:4: note: in definition of macro >> ‘__compiletime_assert’ >> prefix ## suffix();\ >> ^ >> include/linux/compiler.h:447:2: note: in expansion of macro >> ‘_compiletime_assert’ >>_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) >>^ >> include/linux/bug.h:50:37: note: in expansion of macro ‘compiletime_assert’ >> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) >> ^ >> include/linux/bug.h:74:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ >>BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) >>^ >> include/linux/netdevice.h:2016:2: note: in expansion of macro ‘BUILD_BUG_ON’ >>BUILD_BUG_ON(sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN); >>^ >> scripts/Makefile.build:264: recipe for target >> 'drivers/net/ethernet/broadcom/bcmsysport.o' failed >> > >
Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion
On Thu, 2015-07-30 at 18:51 -0700, Florian Fainelli wrote: > On 30/07/15 15:51, David Miller wrote: > > From: David Miller > > Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT) > > > >> This looks fine, series applied, thanks. > > > > I think your control block is too large, you'll need to rework this > > somehow. > > So napi_gro_cb really is 48 bytes on 64-bits architectures (had not > realized it was so big). > > What would you recommend to do here considering that this driver is > currently used on 32-bits platforms, but I see no reason why someone > would no want to use this feature on a 64-bit platform, yet we are > competing with napi_gro_cb, and adding a new skbuff member is pretty > much a no-no? Would it be acceptable to have a new member which is ifdef > CONFIG_NET_DSA_TAG_BRCM? > > FWIW, this does provide a small 2-3% throughput increase for RX. Which layer will read this tag after GRO processing ? It seems you simply can use skb->cb[] like other layers : At offset 0. BTW brcm_tag_rcv() does not even use GRO, as it calls netif_receive_skb() -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion
On 30/07/15 23:08, Eric Dumazet wrote: > On Thu, 2015-07-30 at 18:51 -0700, Florian Fainelli wrote: >> On 30/07/15 15:51, David Miller wrote: >>> From: David Miller >>> Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT) >>> This looks fine, series applied, thanks. >>> >>> I think your control block is too large, you'll need to rework this >>> somehow. >> >> So napi_gro_cb really is 48 bytes on 64-bits architectures (had not >> realized it was so big). >> >> What would you recommend to do here considering that this driver is >> currently used on 32-bits platforms, but I see no reason why someone >> would no want to use this feature on a 64-bit platform, yet we are >> competing with napi_gro_cb, and adding a new skbuff member is pretty >> much a no-no? Would it be acceptable to have a new member which is ifdef >> CONFIG_NET_DSA_TAG_BRCM? >> >> FWIW, this does provide a small 2-3% throughput increase for RX. > > Which layer will read this tag after GRO processing ? DSA would read this tag, but in general any ethertype hook using packet_type would be in the same boat. > > It seems you simply can use skb->cb[] like other layers : At offset 0. > > BTW brcm_tag_rcv() does not even use GRO, as it calls > netif_receive_skb() That is right here, we will come from the RX processing of a driver that might use napi_gro_receive, as it turns out the sequence looks like this here: bcm_sysport_desc_rx() -> extract tag eth_type_trans() sets skb->protocol to ETH_P_XDSA napi_gro_receive(), sets skb->cb to napi_gro_cb -> walk the list of packet_type find the one for ETH_P_XDSA -> brcm_tag_rcv() BTW, there is no build time assertion that napi_gro_cb is not exceeding skb->cb right now, even though they both have the same size on 64-bits, should we have one? -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion
From: Florian Fainelli Date: Thu, 30 Jul 2015 18:51:57 -0700 > On 30/07/15 15:51, David Miller wrote: >> From: David Miller >> Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT) >> >>> This looks fine, series applied, thanks. >> >> I think your control block is too large, you'll need to rework this >> somehow. > > So napi_gro_cb really is 48 bytes on 64-bits architectures (had not > realized it was so big). > > What would you recommend to do here considering that this driver is > currently used on 32-bits platforms, but I see no reason why someone > would no want to use this feature on a 64-bit platform, yet we are > competing with napi_gro_cb, and adding a new skbuff member is pretty > much a no-no? Would it be acceptable to have a new member which is ifdef > CONFIG_NET_DSA_TAG_BRCM? > > FWIW, this does provide a small 2-3% throughput increase for RX. You really need to find a way to make it fit in it's current size on 64-bit. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
