Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[Chris Packham <Chris.Packham@alliedtelesis.co.nz>]

Hi Florian,

On 07/31/2015 01:51 PM, Florian Fainelli wrote:
> On 30/07/15 15:51, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> 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
>>
>
>