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

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.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>

---
 include/linux/if_vlan.h |  1 +
 net/8021q/vlan_core.c   | 10 ++++++++++
 net/8021q/vlan_dev.c    | 26 ++++++++++++++++++++++++++
 3 files changed, 37 insertions(+)

-- 
2.17.1

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 seems to me that what you need may be something like either:

>

>- notifications on slave devices when addresses are added via

>ndo_set_rxmode()

>

>or

>

>- dev_{uc,mc}_sync() should be augmented with a "source net_device"

>argument which allows you to differentiate which network device is the

>source of the address programming. That way, no need to "hash" the MAC

>address with a VID, any network device specific information can be

>provided and in the real device driver you can do: if

>(netif_is_vlan()... etc.)

No issue to retrieve vlan dev if it's directly on top of real dev.
Issue is to get it when it's not directly connected as it's not in
vlan_info group list. Who knows what else can be "structed" on top of
real dev till the vlan device. Please look on reply for cover letter,
as it seems requires similar response.

>

>Hopefully someone else will chime in.

>

>>

>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>

>> ---

>>  include/linux/if_vlan.h |  1 +

>>  net/8021q/vlan_core.c   | 10 ++++++++++

>>  net/8021q/vlan_dev.c    | 26 ++++++++++++++++++++++++++

>>  3 files changed, 37 insertions(+)

>>

>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h

>> index 4cca4da7a6de..94657f3c483a 100644

>> --- a/include/linux/if_vlan.h

>> +++ b/include/linux/if_vlan.h

>> @@ -136,6 +136,7 @@ extern struct net_device *__vlan_find_dev_deep_rcu(struct net_device *real_dev,

>>  extern int vlan_for_each(struct net_device *dev,

>>  			 int (*action)(struct net_device *dev, int vid,

>>  				       void *arg), void *arg);

>> +extern u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8 *addr);

>>  extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);

>>  extern u16 vlan_dev_vlan_id(const struct net_device *dev);

>>  extern __be16 vlan_dev_vlan_proto(const struct net_device *dev);

>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c

>> index a313165e7a67..5d17947d6988 100644

>> --- a/net/8021q/vlan_core.c

>> +++ b/net/8021q/vlan_core.c

>> @@ -454,6 +454,16 @@ bool vlan_uses_dev(const struct net_device *dev)

>>  }

>>  EXPORT_SYMBOL(vlan_uses_dev);

>>

>> +u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8 *addr)

>> +{

>> +	u16 vid = 0;

>> +

>> +	vid = addr[dev->addr_len];

>> +	vid |= (addr[dev->addr_len + 1] & 0xf) << 8;

>> +	return vid;

>> +}

>> +EXPORT_SYMBOL(vlan_dev_get_addr_vid);

>> +

>>  static struct sk_buff *vlan_gro_receive(struct list_head *head,

>>  					struct sk_buff *skb)

>>  {

>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c

>> index b2d9c8f27cd7..c05b313314b7 100644

>> --- a/net/8021q/vlan_dev.c

>> +++ b/net/8021q/vlan_dev.c

>> @@ -250,6 +250,14 @@ void vlan_dev_get_realdev_name(const struct net_device *dev, char *result)

>>  	strncpy(result, vlan_dev_priv(dev)->real_dev->name, 23);

>>  }

>>

>> +static void vlan_dev_set_addr_vid(struct net_device *vlan_dev, u8 *addr)

>> +{

>> +	u16 vid = vlan_dev_vlan_id(vlan_dev);

>> +

>> +	addr[vlan_dev->addr_len] = vid & 0xff;

>> +	addr[vlan_dev->addr_len + 1] = (vid >> 8) & 0xf;

>> +}

>> +

>>  bool vlan_dev_inherit_address(struct net_device *dev,

>>  			      struct net_device *real_dev)

>>  {

>> @@ -481,8 +489,26 @@ static void vlan_dev_change_rx_flags(struct net_device *dev, int change)

>>  	}

>>  }

>>

>> +static void vlan_dev_align_addr_vid(struct net_device *vlan_dev)

>> +{

>> +	struct net_device *real_dev = vlan_dev_real_dev(vlan_dev);

>> +	struct netdev_hw_addr *ha;

>> +

>> +	if (!real_dev->vid_len)

>> +		return;

>> +

>> +	netdev_for_each_mc_addr(ha, vlan_dev)

>> +		if (!ha->sync_cnt)

>> +			vlan_dev_set_addr_vid(vlan_dev, ha->addr);

>> +

>> +	netdev_for_each_uc_addr(ha, vlan_dev)

>> +		if (!ha->sync_cnt)

>> +			vlan_dev_set_addr_vid(vlan_dev, ha->addr);

>> +}

>> +

>>  static void vlan_dev_set_rx_mode(struct net_device *vlan_dev)

>>  {

>> +	vlan_dev_align_addr_vid(vlan_dev);

>>  	dev_mc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);

>>  	dev_uc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);

>>  }

>>

>

>

>-- 

>Florian


-- 
Regards,
Ivan Khoronzhuk

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.

> 

>>

>> It seems to me that what you need may be something like either:

>>

>> - notifications on slave devices when addresses are added via

>> ndo_set_rxmode()

>>

>> or

>>

>> - dev_{uc,mc}_sync() should be augmented with a "source net_device"

>> argument which allows you to differentiate which network device is the

>> source of the address programming. That way, no need to "hash" the MAC

>> address with a VID, any network device specific information can be

>> provided and in the real device driver you can do: if

>> (netif_is_vlan()... etc.)

> No issue to retrieve vlan dev if it's directly on top of real dev.

> Issue is to get it when it's not directly connected as it's not in

> vlan_info group list. Who knows what else can be "structed" on top of

> real dev till the vlan device. Please look on reply for cover letter,

> as it seems requires similar response.


In that case, there are notifications generated that you must be
listening to determine whether you have something like a VLAN device on
top of a bond, which is a port member of a bridge, on which one of your
real device port is enslaved (yes, it can be that type of stacking).

> 

>>

>> Hopefully someone else will chime in.

>>

>>>

>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>

>>> ---

>>>  include/linux/if_vlan.h |  1 +

>>>  net/8021q/vlan_core.c   | 10 ++++++++++

>>>  net/8021q/vlan_dev.c    | 26 ++++++++++++++++++++++++++

>>>  3 files changed, 37 insertions(+)

>>>

>>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h

>>> index 4cca4da7a6de..94657f3c483a 100644

>>> --- a/include/linux/if_vlan.h

>>> +++ b/include/linux/if_vlan.h

>>> @@ -136,6 +136,7 @@ extern struct net_device

>>> *__vlan_find_dev_deep_rcu(struct net_device *real_dev,

>>>  extern int vlan_for_each(struct net_device *dev,

>>>               int (*action)(struct net_device *dev, int vid,

>>>                         void *arg), void *arg);

>>> +extern u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8

>>> *addr);

>>>  extern struct net_device *vlan_dev_real_dev(const struct net_device

>>> *dev);

>>>  extern u16 vlan_dev_vlan_id(const struct net_device *dev);

>>>  extern __be16 vlan_dev_vlan_proto(const struct net_device *dev);

>>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c

>>> index a313165e7a67..5d17947d6988 100644

>>> --- a/net/8021q/vlan_core.c

>>> +++ b/net/8021q/vlan_core.c

>>> @@ -454,6 +454,16 @@ bool vlan_uses_dev(const struct net_device *dev)

>>>  }

>>>  EXPORT_SYMBOL(vlan_uses_dev);

>>>

>>> +u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8 *addr)

>>> +{

>>> +    u16 vid = 0;

>>> +

>>> +    vid = addr[dev->addr_len];

>>> +    vid |= (addr[dev->addr_len + 1] & 0xf) << 8;

>>> +    return vid;

>>> +}

>>> +EXPORT_SYMBOL(vlan_dev_get_addr_vid);

>>> +

>>>  static struct sk_buff *vlan_gro_receive(struct list_head *head,

>>>                      struct sk_buff *skb)

>>>  {

>>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c

>>> index b2d9c8f27cd7..c05b313314b7 100644

>>> --- a/net/8021q/vlan_dev.c

>>> +++ b/net/8021q/vlan_dev.c

>>> @@ -250,6 +250,14 @@ void vlan_dev_get_realdev_name(const struct

>>> net_device *dev, char *result)

>>>      strncpy(result, vlan_dev_priv(dev)->real_dev->name, 23);

>>>  }

>>>

>>> +static void vlan_dev_set_addr_vid(struct net_device *vlan_dev, u8

>>> *addr)

>>> +{

>>> +    u16 vid = vlan_dev_vlan_id(vlan_dev);

>>> +

>>> +    addr[vlan_dev->addr_len] = vid & 0xff;

>>> +    addr[vlan_dev->addr_len + 1] = (vid >> 8) & 0xf;

>>> +}

>>> +

>>>  bool vlan_dev_inherit_address(struct net_device *dev,

>>>                    struct net_device *real_dev)

>>>  {

>>> @@ -481,8 +489,26 @@ static void vlan_dev_change_rx_flags(struct

>>> net_device *dev, int change)

>>>      }

>>>  }

>>>

>>> +static void vlan_dev_align_addr_vid(struct net_device *vlan_dev)

>>> +{

>>> +    struct net_device *real_dev = vlan_dev_real_dev(vlan_dev);

>>> +    struct netdev_hw_addr *ha;

>>> +

>>> +    if (!real_dev->vid_len)

>>> +        return;

>>> +

>>> +    netdev_for_each_mc_addr(ha, vlan_dev)

>>> +        if (!ha->sync_cnt)

>>> +            vlan_dev_set_addr_vid(vlan_dev, ha->addr);

>>> +

>>> +    netdev_for_each_uc_addr(ha, vlan_dev)

>>> +        if (!ha->sync_cnt)

>>> +            vlan_dev_set_addr_vid(vlan_dev, ha->addr);

>>> +}

>>> +

>>>  static void vlan_dev_set_rx_mode(struct net_device *vlan_dev)

>>>  {

>>> +    vlan_dev_align_addr_vid(vlan_dev);

>>>      dev_mc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);

>>>      dev_uc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);

>>>  }

>>>

>>

>>

>> -- 

>> Florian

> 



-- 
Florian

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.

>

>>

>> I was thinking also about pinned list of vlans to the address, but in

>> this case this information also has to be synced by members of device

>> chain,

>> because it can be modified on any device level and it looks not very

>> friendly,

>> and at the end address space has addresses with pinned lists of vlans with

>> their pointers. But keeping this stuff in sync is not simplest decision.

>>

>>

>

>I really think we are not communicating properly, it really seems to me

>that if you had the information about the upper device trying to add an

>address to the lower device filter's either through notification or call

>to ndo_set_rxmode, you could be solving your problems. What are we

>missing here?

>--

>Florian


-- 
Regards,
Ivan Khoronzhuk

On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote:

...

>>

>> I was thinking also about pinned list of vlans to the address, but in

>> this case this information also has to be synced by members of device

>> chain,

>> because it can be modified on any device level and it looks not very

>> friendly,

>> and at the end address space has addresses with pinned lists of vlans with

>> their pointers. But keeping this stuff in sync is not simplest decision.

>>

>>

>

>I really think we are not communicating properly, it really seems to me

>that if you had the information about the upper device trying to add an

>address to the lower device filter's either through notification or call

>to ndo_set_rxmode, you could be solving your problems. What are we

>missing here?


Sry, missed this one. The problem in getting  the owner of address.
Just simple case: vlan/macvlan/real_dev or vlan/.../.../real_dev

The real dev hasn't simple way to get vid the address belong to, or it has?



>--

>Florian


-- 
Regards,
Ivan Khoronzhuk

On 1/7/19 9:01 PM, Florian Fainelli wrote:
> Le 12/4/18 à 4:04 PM, Ivan Khoronzhuk a écrit :

>> On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote:

>>

>> ...

>>

>>>>

>>>> I was thinking also about pinned list of vlans to the address, but in

>>>> this case this information also has to be synced by members of device

>>>> chain,

>>>> because it can be modified on any device level and it looks not very

>>>> friendly,

>>>> and at the end address space has addresses with pinned lists of vlans

>>>> with

>>>> their pointers. But keeping this stuff in sync is not simplest decision.

>>>>

>>>>

>>>

>>> I really think we are not communicating properly, it really seems to me

>>> that if you had the information about the upper device trying to add an

>>> address to the lower device filter's either through notification or call

>>> to ndo_set_rxmode, you could be solving your problems. What are we

>>> missing here?

>>

>> Sry, missed this one. The problem in getting  the owner of address.

>> Just simple case: vlan/macvlan/real_dev or vlan/.../.../real_dev

>>

>> The real dev hasn't simple way to get vid the address belong to, or it has?

> 

> Humm looks like your right, by the time the address lists are

> synchronized (e.g: from = vlan_dev, to = real_dev), we lost that

> information. It looks like I just managed to find such an use case

> myself with VLAN filtering enabled on a bridge (so switch is VLAN aware)

> and a VLAN device created on the bridge (br0.42) but with IGMP snooping

> turned off (so we don't get HOST_MDB notifications with correct VLAN ID).

> 

> Maybe keeping the "from" net_device within the address list that is

> processed by ndo_set_rx_mode() will do the job though?

> 

> Then you can do things like:

> 

> if (is_vlan_dev(ha->dev) && ha->dev != dev)

> 	vid = vlan_dev_vlan_id(ha->dev);

> 

> and it should scale to any type of stacked device, regardless of VID or

> something else that we need?

> 

> Can you remind me of your use case again? Is it because your switch has

> VLAN filtering enabled and you need to make sure that MC addresses on

> VLAN device get programmed into the switch's multicast database with

> correct VID?


Ivan, can you see if the following would work for you:

https://github.com/ffainelli/linux/commit/19e173ebdcdd32f5f5b5ef29049e35d33ad058f2

this should be more scalable approach in that we can support HOST_MDB
notifications from the bridge, the same way we would get notified about
IGMP snooping from the bridge and this does not impact any other driver
than those that elect to receive switchdev object notifications, which
cpsw should really implement by now...
-- 
Florian

On Tue, Jan 08, 2019 at 10:21:25AM -0800, Florian Fainelli wrote:
>On 1/7/19 9:01 PM, Florian Fainelli wrote:

>> Le 12/4/18 à 4:04 PM, Ivan Khoronzhuk a écrit :

>>> On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote:

>>>

>>> ...

>>>

>>>>>

>>>>> I was thinking also about pinned list of vlans to the address, but in

>>>>> this case this information also has to be synced by members of device

>>>>> chain,

>>>>> because it can be modified on any device level and it looks not very

>>>>> friendly,

>>>>> and at the end address space has addresses with pinned lists of vlans

>>>>> with

>>>>> their pointers. But keeping this stuff in sync is not simplest decision.

>>>>>

>>>>>

>>>>

>>>> I really think we are not communicating properly, it really seems to me

>>>> that if you had the information about the upper device trying to add an

>>>> address to the lower device filter's either through notification or call

>>>> to ndo_set_rxmode, you could be solving your problems. What are we

>>>> missing here?

>>>

>>> Sry, missed this one. The problem in getting  the owner of address.

>>> Just simple case: vlan/macvlan/real_dev or vlan/.../.../real_dev

>>>

>>> The real dev hasn't simple way to get vid the address belong to, or it has?

>>

>> Humm looks like your right, by the time the address lists are

>> synchronized (e.g: from = vlan_dev, to = real_dev), we lost that

>> information. It looks like I just managed to find such an use case

>> myself with VLAN filtering enabled on a bridge (so switch is VLAN aware)

>> and a VLAN device created on the bridge (br0.42) but with IGMP snooping

>> turned off (so we don't get HOST_MDB notifications with correct VLAN ID).

>>

>> Maybe keeping the "from" net_device within the address list that is

>> processed by ndo_set_rx_mode() will do the job though?

>>

>> Then you can do things like:

>>

>> if (is_vlan_dev(ha->dev) && ha->dev != dev)

>> 	vid = vlan_dev_vlan_id(ha->dev);

>>

>> and it should scale to any type of stacked device, regardless of VID or

>> something else that we need?

>>

>> Can you remind me of your use case again? Is it because your switch has

>> VLAN filtering enabled and you need to make sure that MC addresses on

>> VLAN device get programmed into the switch's multicast database with

>> correct VID?

>

>Ivan, can you see if the following would work for you:

>

>https://github.com/ffainelli/linux/commit/19e173ebdcdd32f5f5b5ef29049e35d33ad058f2

>

>this should be more scalable approach in that we can support HOST_MDB

>notifications from the bridge, the same way we would get notified about

>IGMP snooping from the bridge and this does not impact any other driver

>than those that elect to receive switchdev object notifications, which

>cpsw should really implement by now...


Sorry missed 2 last comments and seems like it's clear now.
I need to be in TO or CC list my filters can parse it, probably I need
to create more smart filters.

>-- 

>Florian


-- 
Regards,
Ivan Khoronzhuk

On February 13, 2019 8:17:16 AM PST, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>On Tue, Jan 22, 2019 at 03:12:41PM +0200, Ivan Khoronzhuk wrote:

>>On Mon, Jan 21, 2019 at 03:37:41PM -0800, Florian Fainelli wrote:

>>>On 12/4/18 3:42 PM, Ivan Khoronzhuk wrote:

>>>>On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote:

>>

>>[...]

>>

>>>

>>>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!

>>

>>Yes, sure. I can start to do that in several days.

>>Just a little busy right now.

>>

>>Just before doing this, maybe some comments could be added as it has

>more

>>attention now. Meanwhile I can send alternative variant but based on

>>real dev splitting addresses between vlans. In this approach it leaves

>address

>>space w/o vid extension but requires more changes to vlan core.

>Drawback here

>>that to change one address alg traverses all related vlan addresses,

>it can be

>>cpu/time wasteful, if it's done regularly, but saves memory....

>>

>>Basically it's implemented locally in cpsw and requires more changes

>to move

>>it as some vlan core auxiliary functions to be reused. But it can work

>only

>>with vlans directly on top of real dev, which is fixable.

>>

>>Core function here:

>>__hw_addr_ref_sync_dev

>>it is called only for address the link of which was

>increased/decreased, thus

>>update made only on one address, comparing it for every vlan dev.

>>

>>It was added with this patch:

>>[1] net: core: dev_addr_lists: add auxiliary func to handle reference 

>>address update e7946760de5852f32

>>

>>And used by this patch:

>>[2] net: ethernet: ti: cpsw: fix vlan mcast 15180eca569bfe1d4d

>>

>>So, idea is to move [2] to be vlan core auxiliary function to be

>reused

>>by NIC drivers.

>>

>>But potentially it can bring a little more changes I assume:

>>

>>1) add priv_flag |= IFF_IV_FLT (independent vlan filtering). It allows

>to reuse

>>this flag for farther changes, probably for per vlan allmulti or so.

>>

>>2) real dev has to have complete list for vlans, not only their vids,

>but also

>>all vlandevs in device chain above it. So changes in add_vid can be

>required.

>>Vlan core can assign vlan dev pointer to real device only after it's

>completely

>>initialized. And for propagation reasons it requires every device in

>>infrastructure to be aware. That seems doable, but depends not only on

>me.

>>

>>3) Move code from [2] to be auxiliary vlan core API for setting mc and

>uc.

>>From this patch only one function is cpsw specific: cpsw_set_mc(). The

>rest can

>>be applicable on every NIC supporting IFF_IV_FLT.

>>

>>4) Move code from link below to do the same but for uc addresses:

>>https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/commit/?h=ucast_vlan_fix&id=ebc88a7d8758759322d9ff88f25f8bac51ce7219

>>here only one func cpsw specific: cpsw_set_uc()

>>the rest can be generic.

>>

>>As third alternative, we can think about how to reduce memory for

>addresses by

>>reusing them or else, but this is as continuation of addr+vid

>approach, and API

>>probably would be the same.

>>

>>Then all this can be compared for proper decision.

>

>

>Hi Florian,

>

>After several more investigations and tries probably better left this

>idea as is.


Thank you for keeping the thread alive, does that mean you are going to resubmit this patch series as-is (rebased) or are you saying that you are abandoning the idea and leaving the situation the way it is in cpsw?

>

>Here actually several explanations for this:

>1) If even assume that we can get access to vlan devices in the above

>ndev

>tree (we can) that doesn't guarantee that receive vlan filters are set

>replicating this structure. For example bond device can have one active

>slave

>but both of them in the tree having vid set, in this case addresses are

>syched only with active slave, no filters should be applied to not

>active slave.

>this can be achieved only each address has vid context.

>

>2) According to 1) rx filters device structure can be created while

>mc_sync()

>in each rx_mode(), and then used as orthogonal info. I've tried and it

>looks

>not cool and consumes anyway memory and even if it's less it's still

>not very

>scalable. (+ no normal signal "in complex structure case" when address

>should

>be undated to avoid redundant cpu cycles). Not sure it can have

>practical

>results and be universal enouph.

>

>3) Assuming that every device in the tree (bond, team or else) is legal

>to

>modify its own address space, the real end device cannot be sure the

>vlan device

>address spaces reflects vid addresses that device tree want's from him.

>According to this each address in address space must hold its own

>context at

>every device and this context is comparable with address size.

>

>>-- Regards,

>>Ivan Khoronzhuk


-- 
Florian

On Wed, Feb 13, 2019 at 08:49:39PM -0800, Florian Fainelli wrote:
>

>

>On February 13, 2019 8:17:16 AM PST, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:

>>On Tue, Jan 22, 2019 at 03:12:41PM +0200, Ivan Khoronzhuk wrote:

>>>On Mon, Jan 21, 2019 at 03:37:41PM -0800, Florian Fainelli wrote:

>>>>On 12/4/18 3:42 PM, Ivan Khoronzhuk wrote:

>>>>>On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote:

>>>

>>>[...]

>>>

>>>>

>>>>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!

>>>

>>>Yes, sure. I can start to do that in several days.

>>>Just a little busy right now.

>>>

>>>Just before doing this, maybe some comments could be added as it has

>>more

>>>attention now. Meanwhile I can send alternative variant but based on

>>>real dev splitting addresses between vlans. In this approach it leaves

>>address

>>>space w/o vid extension but requires more changes to vlan core.

>>Drawback here

>>>that to change one address alg traverses all related vlan addresses,

>>it can be

>>>cpu/time wasteful, if it's done regularly, but saves memory....

>>>

>>>Basically it's implemented locally in cpsw and requires more changes

>>to move

>>>it as some vlan core auxiliary functions to be reused. But it can work

>>only

>>>with vlans directly on top of real dev, which is fixable.

>>>

>>>Core function here:

>>>__hw_addr_ref_sync_dev

>>>it is called only for address the link of which was

>>increased/decreased, thus

>>>update made only on one address, comparing it for every vlan dev.

>>>

>>>It was added with this patch:

>>>[1] net: core: dev_addr_lists: add auxiliary func to handle reference

>>>address update e7946760de5852f32

>>>

>>>And used by this patch:

>>>[2] net: ethernet: ti: cpsw: fix vlan mcast 15180eca569bfe1d4d

>>>

>>>So, idea is to move [2] to be vlan core auxiliary function to be

>>reused

>>>by NIC drivers.

>>>

>>>But potentially it can bring a little more changes I assume:

>>>

>>>1) add priv_flag |= IFF_IV_FLT (independent vlan filtering). It allows

>>to reuse

>>>this flag for farther changes, probably for per vlan allmulti or so.

>>>

>>>2) real dev has to have complete list for vlans, not only their vids,

>>but also

>>>all vlandevs in device chain above it. So changes in add_vid can be

>>required.

>>>Vlan core can assign vlan dev pointer to real device only after it's

>>completely

>>>initialized. And for propagation reasons it requires every device in

>>>infrastructure to be aware. That seems doable, but depends not only on

>>me.

>>>

>>>3) Move code from [2] to be auxiliary vlan core API for setting mc and

>>uc.

>>>From this patch only one function is cpsw specific: cpsw_set_mc(). The

>>rest can

>>>be applicable on every NIC supporting IFF_IV_FLT.

>>>

>>>4) Move code from link below to do the same but for uc addresses:

>>>https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/commit/?h=ucast_vlan_fix&id=ebc88a7d8758759322d9ff88f25f8bac51ce7219

>>>here only one func cpsw specific: cpsw_set_uc()

>>>the rest can be generic.

>>>

>>>As third alternative, we can think about how to reduce memory for

>>addresses by

>>>reusing them or else, but this is as continuation of addr+vid

>>approach, and API

>>>probably would be the same.

>>>

>>>Then all this can be compared for proper decision.

>>

>>

>>Hi Florian,

>>

>>After several more investigations and tries probably better left this

>>idea as is.

>

>Thank you for keeping the thread alive, does that mean you are going to resubmit this patch series as-is (rebased) or are you saying that you are abandoning the idea and leaving the situation the way it is in cpsw?


I will resubmit this one. But:

I have to try one more approach before this.
The idea is to create simple rx flt device tree while mc/us sync.
Then use it at real device to dispatch addresses.

It increases hw_addr struct a little and code base,
But:
- no need to keep linearly all vlan addresses in one address space.
- replicates RX filtering struct of net devices,
  (but not logical tree of netdevs)
- keeps devs info per address.
- no need to change addr lenth and modify existent API
- access at any net dev to above rx flt device structure per address
- potentially can be use not only for vlan devs identification but for
  other rx path offloads.

Idea is simple but not completely verified it yet,
need a little bit more time to prove/clean ...or drop it.

>

>>

>>Here actually several explanations for this:

>>1) If even assume that we can get access to vlan devices in the above

>>ndev

>>tree (we can) that doesn't guarantee that receive vlan filters are set

>>replicating this structure. For example bond device can have one active

>>slave

>>but both of them in the tree having vid set, in this case addresses are

>>syched only with active slave, no filters should be applied to not

>>active slave.

>>this can be achieved only each address has vid context.

>>

>>2) According to 1) rx filters device structure can be created while

>>mc_sync()

>>in each rx_mode(), and then used as orthogonal info. I've tried and it

>>looks

>>not cool and consumes anyway memory and even if it's less it's still

>>not very

>>scalable. (+ no normal signal "in complex structure case" when address

>>should

>>be undated to avoid redundant cpu cycles). Not sure it can have

>>practical

>>results and be universal enouph.

>>

>>3) Assuming that every device in the tree (bond, team or else) is legal

>>to

>>modify its own address space, the real end device cannot be sure the

>>vlan device

>>address spaces reflects vid addresses that device tree want's from him.

>>According to this each address in address space must hold its own

>>context at

>>every device and this context is comparable with address size.

>>

>>>-- Regards,

>>>Ivan Khoronzhuk

>

>-- 

>Florian


-- 
Regards,
Ivan Khoronzhuk