Re: Hardcoded multicast queue length in macvlan.c driver causes poor multicast receive performance

From: Thomas Karlsson <thomas.karlsson@paneda.se>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	<thomas.karlsson@paneda.se>
Subject: Re: Hardcoded multicast queue length in macvlan.c driver causes poor multicast receive performance
Date: Wed, 25 Nov 2020 23:15:39 +0100	[thread overview]
Message-ID: <956c4fca-2a54-97cb-5b4c-3a286743884b@paneda.se> (raw)
In-Reply-To: <20201125100710.7e766d7e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

On 2020-11-25 19:07, Jakub Kicinski wrote:
> On Wed, 25 Nov 2020 18:12:34 +0100 Thomas Karlsson wrote:
>>>> For this reason I would like to know if you would consider
>>>> merging a patch using the module_param(...) variant instead?
>>>>
>>>> I would argue that this still makes the situation better
>>>> and resolves the packet-loss issue, although not necessarily
>>>> in an optimal way. However, The upside of being able to specify the
>>>> parameter on a per macvlan interface level instead of globally is not
>>>> that big in this situation. Normally you don't use that much
>>>> multicast anyway so it's a parameter that only will be touched by
>>>> a very small user base that can understand and handle the implications
>>>> of such a global setting.  
>>>
>>> How about implementing .changelink in macvlan? That way you could
>>> modify the macvlan device independent of Docker? 
>>>
>>> Make sure you only accept changes to the bc queue len if that's the
>>> only one you act on.
>>>   
>>
>> Hmm, I see. You mean that docker can create the interface and then I can
>> modify it afterwards? That might be a workaround but I just submitted
>> a patch (like seconds before your message) with the module_param() option
>> and this was very clean I think. both in how little code that needed to be
>> changed and in how simple it is to set the option in the target environment.
>>
>> This is my first time ever attemting a contribution to the kernel so
>> I'm quite happy to keep it simple like that too :)
> 
> Module params are highly inflexible, we have a general policy not 
> to accept them in the netdev world.
> 

I see, although the current define seems even less flexible :)
Although, I might not have fully understood the .changelink you suggest.
Is it via the ip link set ... command? Or is there a way to set the parameters
in a more "raw" form that does not require a patch to iproute2 with parameter parsing,
error handing, man pages updates, etc. I feel that I'm getting in over my head here.

I appreciate your feedback!

> There should even be a check> in our patchwork which should fail here, but it appears that the patch 
> did not apply in the first place:
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/385b9b4c-25f5-b507-4e69-419883fa8043@paneda.se/
> 
> Make sure you're developing on top of this tree:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/
> 

Right, thanks! It's a bit of a learning curve. I had incorrectly done the work on top of torvalds/linux