Re: [RFC PATCH net-next 2/5] net: 8021q: vlan_dev: add vid tag for uc and mc address lists

[Florian Fainelli <f.fainelli@gmail.com>]

On 12/4/18 3:42 PM, Ivan Khoronzhuk wrote:
> On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote:
>> On 12/4/18 10:57 AM, Ivan Khoronzhuk wrote:
>>> On Mon, Dec 03, 2018 at 03:57:03PM -0800, Florian Fainelli wrote:
>>>> On 12/3/18 3:51 PM, Ivan Khoronzhuk wrote:
>>>>> On Mon, Dec 03, 2018 at 02:17:00PM -0800, Florian Fainelli wrote:
>>>>>> On 12/3/18 10:40 AM, Ivan Khoronzhuk wrote:
>>>>>>> Update vlan mc and uc addresses with VID tag while propagating
>>>>>>> address
>>>>>>> set to lower devices, do this only if address is not synched. It
>>>>>>> allows
>>>>>>> on end driver level to distinguish address belonging to vlans.
>>>>>>
>>>>>> Underlying driver for the real device would be able to properly
>>>>>> identify
>>>>>> that you are attempting to add an address to a virtual device, which
>>>>>> happens to be of VLAN kind so I am really not sure this is the right
>>>>>> approach here.
>>>>>>
>>>>>> From there, it seems to me that we have two situations:
>>>>>>
>>>>>> - each of your network devices expose VLAN devices directly on top of
>>>>>> the real device, in which case your driver should support
>>>>>> ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid to know when VLAN
>>>>>> devices
>>>>>> are create and maintain a VLAN device to VID correspondence if it
>>>>>> needs
>>>>>> to when being called while setting the addresses
>>>>>>
>>>>>> - you are setting up a bridge that is VLAN aware on one of your
>>>>>> bridge
>>>>>> ports, and there you can use switchdev to learn about such events and
>>>>>> know about both addresses as well as VIDs that must be programmed
>>>>>> into
>>>>>> your real device
>>>>> No limits to have any "middle" device between real end device and
>>>>> virtual one, not only a bridge, but also other kind. And as it's
>>>>> generic
>>>>> change, it should cover all such cases, the simplest example is:
>>>>> real_dev/macvlan/vlan.
>>>>
>>>> It is not generic if the additional information is a VLAN ID, that
>>>> construct does not apply to all types of virtual devices, that is part
>>>> of my issue with the extra VID that is being added. If this was a
>>>> void *
>>>> priv and any virtual device could pass up/down information that
>>>> might be
>>>> more acceptable.
>>>
>>> You mean to create smth like common struct pinned to "an address" and
>>> pass information not only like vid, but in parallel what ever user
>>> wanted.
>>> Even if pass vlan device pointer it still considered like an address
>>> continuation and same sync method is used w/o modification. And here vid
>>> is considered as part of address, by a big account address+vid it's a
>>> separate address, same happens with the pointer, address+pointer it's
>>> still separate address.
>>
>> That depends on the HW implementation, some switches do individual VLAN
>> learning (IVL) and some do shared VLAN learning (SVL) so whether the VID
>> becomes part of the address resolution logic is HW dependent, obviously
>> the more capable, the better (IVL).
> 
> In my case IVL is only choice, as SVL is rather imitation, as each vlan
> has it's own address table anyway. And I mean not only vlan configuration
> above the bridge but also any simple configuration above real device.
> 
> There is proposition to add smth like additional list of entries pinned
> to the each address as you proposed, but in a little bit different way.
> 
> Pin the context pointers to each address if IVL config is enabled.
> Smth like
> 
> +struct ctx_entry {
> +       void *info;
> +       unsigned type;
> +       int synced;
> +       int sync_cnt;
> +       int refcount;
> +}
> 
> Then in hw_addr struct add a ctx_list:
> 
> struct netdev_hw_addr {
>        struct list_head        list;
> +       struct list_head        ctx_list;
>        unsigned char           addr[MAX_ADDR_LEN];
>        unsigned char           type;
> .....
> }
> 
> Each ctx_entry contains pointer to some structure, in my case it could be
> pointer on vlan net_dev, and it can be marked with type
> VLAN_DEVICE_POINTER or
> else. In some other invisible cases it could be another one. Main
> difference
> between each of them is its pointer and type.
> 
> And once each net dev calls mc/uc_sync these entires, if not synced, are
> synced
> along with addresses. But main difference that these ctx entires are
> pinned to
> the address, when addresses are pinned to the device.
> 
> It can allow to bring information any new abstraction can apply.
> For real device the list can be empty or contain special entry to differ
> it from the vlan device entries, as could happen only some vlan is address
> owner.
> 
> Not sure it can be much simpler but it definitely can introduce more
> capabilities, and potentially cover some other cases including your.
> 
> Probably I need rename the series on smth like:
> "make addressing scheme to be IVL capable"
> 
> and send RFCv2
> 
> Thanks for your comment, it's really valuable.

Ivan, based on the recent submission I copied you on [1], it sounds like
we want to move ahead with your proposal to extend netdev_hw_addr with a
vid member.

On second thought, your approach is good and if we enclose the vid
member within an #if IS_ENABLED(CONFIG_VLAN)8021Q) we should be good for
most foreseeable use cases, if not, we can always introduce a variable
size/defined context in the future.

Can you resubmit this patch series as non-RFC in the next few days so I
can also repost mine [1] and take advantage of these changes for
multicast over VLAN when VLAN filtering is globally enabled on the device.

[1]: https://www.spinics.net/lists/netdev/msg544722.html

Thanks!
-- 
Florian