diff mbox series

[net-next,4/6] ethernet: eth: add default vid len for all ehternet kind devices

Message ID 20190226184556.16082-5-ivan.khoronzhuk@linaro.org (mailing list archive)
State New, archived
Headers show
Series net: add individual virtual device filtering | expand

Commit Message

Ivan Khoronzhuk Feb. 26, 2019, 6:45 p.m. UTC
IVDF - individual virtual device filtering. Allows to set per vlan
l2 address filters on end real network device (for unicast and for
multicast) and drop redundant not expected packet income.

If CONFIG_VLAN_8021Q_IVDF is enabled the following changes are
applied, and only for ethernet network devices.

By default every ethernet netdev needs vid len = 2 bytes to be able to
hold up to 4096 vids. So set it for every eth device to be correct,
except vlan devs.

In order to shrink all addresses of devices above vlan, the vid_len
for vlan dev = 0, as result all suckers sync their addresses to common
base not taking in to account vid part (vid_len of "to" devices is
important only). And only vlan device is the source of addresses with
actual its vid set, propagating it to parent devices while rx_mode().

Also, don't bother those ethernet devices that at this moment are not
moved to vlan addressing scheme, so while end ethernet device is
created - set vid_len to 0, thus, while syncing, its address space is
concatenated to one dimensional like usual, and who needs IVDF - set
it to NET_8021Q_VID_TSIZE.

There is another decision - is to inherit vid_len or some feature flag
from end root device in order to all upper devices have vlan extended
address space only if exact end real device have such capability. But
I didn't, because it requires more changes and probably I'm not
familiar with all places where it should be inherited, I would
appreciate if someone can guid where it's applicable, then it could
become a little bit more limited.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 include/linux/if_vlan.h |  1 +
 net/8021q/Kconfig       | 12 ++++++++++++
 net/8021q/vlan_core.c   | 12 ++++++++++++
 net/8021q/vlan_dev.c    |  1 +
 net/ethernet/eth.c      | 10 ++++++++--
 5 files changed, 34 insertions(+), 2 deletions(-)

Comments

Florian Fainelli Feb. 28, 2019, 4:29 a.m. UTC | #1
On 2/26/2019 10:45 AM, Ivan Khoronzhuk wrote:
> IVDF - individual virtual device filtering. Allows to set per vlan
> l2 address filters on end real network device (for unicast and for
> multicast) and drop redundant not expected packet income.
> 
> If CONFIG_VLAN_8021Q_IVDF is enabled the following changes are
> applied, and only for ethernet network devices.
> 
> By default every ethernet netdev needs vid len = 2 bytes to be able to
> hold up to 4096 vids. So set it for every eth device to be correct,
> except vlan devs.
> 
> In order to shrink all addresses of devices above vlan, the vid_len
> for vlan dev = 0, as result all suckers sync their addresses to common
> base not taking in to account vid part (vid_len of "to" devices is
> important only). And only vlan device is the source of addresses with
> actual its vid set, propagating it to parent devices while rx_mode().
> 
> Also, don't bother those ethernet devices that at this moment are not
> moved to vlan addressing scheme, so while end ethernet device is
> created - set vid_len to 0, thus, while syncing, its address space is
> concatenated to one dimensional like usual, and who needs IVDF - set
> it to NET_8021Q_VID_TSIZE.
> 
> There is another decision - is to inherit vid_len or some feature flag
> from end root device in order to all upper devices have vlan extended
> address space only if exact end real device have such capability. But
> I didn't, because it requires more changes and probably I'm not
> familiar with all places where it should be inherited, I would
> appreciate if someone can guid where it's applicable, then it could
> become a little bit more limited.

I would think that a call to vlan_dev_ivdf_set() would be enough to
indicate that the underlying network device driver supports IVDF and
wants to make use of it. The infrastructure itself that you added costs
little memory, it is once the call to vlan_dev_ivdf_set() is made that
the memory consumption increases which is fine, since we want to make
use of that feature.

While I appreciate the thoughts given to making this a configurable
option, I feel that enabling it unconditionally and having the
underlying driver decide would be more manageable.

We have had that conversation before, but let me ask again when we call
dev_{uc,mc}_sync() and ultimately the network device's
ndo_set_rx_mode(), by the time the ndo_set_rx_mode() function is called,
we lost track of the call chain, like which virtual device was it
originating from. If we somehow added a notification information about
the network device stack (and we could use netdevice notifiers for
that), then maybe we don't really need to add all of this code and we
can just derive the necessary bits of information we want by checking:
is this a VLAN network device? It is, okay what's your VLAN ID, etc.?

Either approach would get us our cookie anyway :)

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

[snip]

> @@ -404,8 +405,13 @@ EXPORT_SYMBOL(ether_setup);
>  struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
>  				      unsigned int rxqs)
>  {
> -	return alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_UNKNOWN,
> -				ether_setup, txqs, rxqs);
> +	struct net_device *dev;
> +
> +	dev = alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_UNKNOWN,
> +			       ether_setup, txqs, rxqs);

You need to check the return value of alloc_netdev_mqs() now, otherwise
you could be doing a NPD in the call right under. Since that is the
default though, do you really need to call vlan_dev_ivdf_set() below?
Ivan Khoronzhuk March 1, 2019, 1:11 p.m. UTC | #2
On Wed, Feb 27, 2019 at 08:29:20PM -0800, Florian Fainelli wrote:
>
>
>On 2/26/2019 10:45 AM, Ivan Khoronzhuk wrote:
>> IVDF - individual virtual device filtering. Allows to set per vlan
>> l2 address filters on end real network device (for unicast and for
>> multicast) and drop redundant not expected packet income.
>>
>> If CONFIG_VLAN_8021Q_IVDF is enabled the following changes are
>> applied, and only for ethernet network devices.
>>
>> By default every ethernet netdev needs vid len = 2 bytes to be able to
>> hold up to 4096 vids. So set it for every eth device to be correct,
>> except vlan devs.
>>
>> In order to shrink all addresses of devices above vlan, the vid_len
>> for vlan dev = 0, as result all suckers sync their addresses to common
>> base not taking in to account vid part (vid_len of "to" devices is
>> important only). And only vlan device is the source of addresses with
>> actual its vid set, propagating it to parent devices while rx_mode().
>>
>> Also, don't bother those ethernet devices that at this moment are not
>> moved to vlan addressing scheme, so while end ethernet device is
>> created - set vid_len to 0, thus, while syncing, its address space is
>> concatenated to one dimensional like usual, and who needs IVDF - set
>> it to NET_8021Q_VID_TSIZE.
>>
>> There is another decision - is to inherit vid_len or some feature flag
>> from end root device in order to all upper devices have vlan extended
>> address space only if exact end real device have such capability. But
>> I didn't, because it requires more changes and probably I'm not
>> familiar with all places where it should be inherited, I would
>> appreciate if someone can guid where it's applicable, then it could
>> become a little bit more limited.
>
>I would think that a call to vlan_dev_ivdf_set() would be enough to
>indicate that the underlying network device driver supports IVDF and
>wants to make use of it. The infrastructure itself that you added costs
>little memory, it is once the call to vlan_dev_ivdf_set() is made that
>the memory consumption increases which is fine, since we want to make
>use of that feature.
>
>While I appreciate the thoughts given to making this a configurable
>option, I feel that enabling it unconditionally and having the
>underlying driver decide would be more manageable.

Not exactly. Even system has no driver calling vlan_dev_ivdf_set()
I still has this "mem consumption" from the very beginning. That's why I made
this depended on the driver and CONF. For embedded world it looks fine.

The issues is that I can't change addressing scheme dynamically since some
drivers and infrastructure that exists before I called vlan_dev_ivdf_set()
can already have some synced addresses using old scheme. To do this
dynamically it needs unsync vlans from old scheme and make sync again.
Probably that is topic to "sync" :-| about....

I considered idea making "above infrastructure" IVDF to be dependent on
underneath end device IVDF and remove this config at all, but here several
issues with this, the infrastructure has to be "resynced" and some signal has
to inform each vlan to do this, and while this happens, all end devices already
configured and not supported IVDF shouldn't suffer. I can try but it looks not
doable in normal way, and appreciate any thoughts about this.

Meanwhile, this option looks fine for small embedded paltforms.

>
>We have had that conversation before, but let me ask again when we call
>dev_{uc,mc}_sync() and ultimately the network device's
>ndo_set_rx_mode(), by the time the ndo_set_rx_mode() function is called,
>we lost track of the call chain, like which virtual device was it
>originating from. If we somehow added a notification information about
>the network device stack (and we could use netdevice notifiers for
>that), then maybe we don't really need to add all of this code and we
>can just derive the necessary bits of information we want by checking:
>is this a VLAN network device? It is, okay what's your VLAN ID, etc.?
>
>Either approach would get us our cookie anyway :)

Postulate here is that address of vlan device is separate from netdevice entity
with it's own context.

Several cases talking about this:

- bound device having 2 slaves can have added vid to both slave devices but
synced addresses for only one of them. So, if vid is set in real device it 
doesn't mean it needs addresses of vlan device.

- I know that's crazy, but net device tree can contain 2 same vlan devices ).
The scheme doesn't prevent this case. So one vid address can be counted by two
vlan network devices.

- Any of the devices in _sync/rx_mode() chain is legal to do with addresses
what ever it's allowed to do, drop some of them, combine with others and more,
even add it's own vid addresses w/o actual vlan network device.

These made me to look in side of building rx_sync netdevice tree holding links
on nodes per each address. And I've did this mostly...then after short look
on this asked myself "who is going to support this ..." and it anyway doesn't
cover all possible cases.

So, lost track of the device looks not so bad and opens more possibilities.

>
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>
>[snip]
>
>> @@ -404,8 +405,13 @@ EXPORT_SYMBOL(ether_setup);
>>  struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
>>  				      unsigned int rxqs)
>>  {
>> -	return alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_UNKNOWN,
>> -				ether_setup, txqs, rxqs);
>> +	struct net_device *dev;
>> +
>> +	dev = alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_UNKNOWN,
>> +			       ether_setup, txqs, rxqs);
>
>You need to check the return value of alloc_netdev_mqs() now, otherwise
>you could be doing a NPD in the call right under. Since that is the
>default though, do you really need to call vlan_dev_ivdf_set() below?

Yep. Has to be done.
But IVDF is not default....and can't be toggled dynamically, at least for now.

>-- 
>Florian
Florian Fainelli March 2, 2019, 3:33 a.m. UTC | #3
On 3/1/2019 5:11 AM, Ivan Khoronzhuk wrote:
> On Wed, Feb 27, 2019 at 08:29:20PM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/26/2019 10:45 AM, Ivan Khoronzhuk wrote:
>>> IVDF - individual virtual device filtering. Allows to set per vlan
>>> l2 address filters on end real network device (for unicast and for
>>> multicast) and drop redundant not expected packet income.
>>>
>>> If CONFIG_VLAN_8021Q_IVDF is enabled the following changes are
>>> applied, and only for ethernet network devices.
>>>
>>> By default every ethernet netdev needs vid len = 2 bytes to be able to
>>> hold up to 4096 vids. So set it for every eth device to be correct,
>>> except vlan devs.
>>>
>>> In order to shrink all addresses of devices above vlan, the vid_len
>>> for vlan dev = 0, as result all suckers sync their addresses to common
>>> base not taking in to account vid part (vid_len of "to" devices is
>>> important only). And only vlan device is the source of addresses with
>>> actual its vid set, propagating it to parent devices while rx_mode().
>>>
>>> Also, don't bother those ethernet devices that at this moment are not
>>> moved to vlan addressing scheme, so while end ethernet device is
>>> created - set vid_len to 0, thus, while syncing, its address space is
>>> concatenated to one dimensional like usual, and who needs IVDF - set
>>> it to NET_8021Q_VID_TSIZE.
>>>
>>> There is another decision - is to inherit vid_len or some feature flag
>>> from end root device in order to all upper devices have vlan extended
>>> address space only if exact end real device have such capability. But
>>> I didn't, because it requires more changes and probably I'm not
>>> familiar with all places where it should be inherited, I would
>>> appreciate if someone can guid where it's applicable, then it could
>>> become a little bit more limited.
>>
>> I would think that a call to vlan_dev_ivdf_set() would be enough to
>> indicate that the underlying network device driver supports IVDF and
>> wants to make use of it. The infrastructure itself that you added costs
>> little memory, it is once the call to vlan_dev_ivdf_set() is made that
>> the memory consumption increases which is fine, since we want to make
>> use of that feature.
>>
>> While I appreciate the thoughts given to making this a configurable
>> option, I feel that enabling it unconditionally and having the
>> underlying driver decide would be more manageable.
> 
> Not exactly. Even system has no driver calling vlan_dev_ivdf_set()
> I still has this "mem consumption" from the very beginning. That's why I
> made
> this depended on the driver and CONF. For embedded world it looks fine.
> 
> The issues is that I can't change addressing scheme dynamically since some
> drivers and infrastructure that exists before I called vlan_dev_ivdf_set()
> can already have some synced addresses using old scheme. To do this
> dynamically it needs unsync vlans from old scheme and make sync again.
> Probably that is topic to "sync" :-| about....

Good one, that would be very complicated indeed.

> 
> I considered idea making "above infrastructure" IVDF to be dependent on
> underneath end device IVDF and remove this config at all, but here several
> issues with this, the infrastructure has to be "resynced" and some
> signal has
> to inform each vlan to do this, and while this happens, all end devices
> already
> configured and not supported IVDF shouldn't suffer. I can try but it
> looks not
> doable in normal way, and appreciate any thoughts about this.
> 
> Meanwhile, this option looks fine for small embedded paltforms.
> 
>>
>> We have had that conversation before, but let me ask again when we call
>> dev_{uc,mc}_sync() and ultimately the network device's
>> ndo_set_rx_mode(), by the time the ndo_set_rx_mode() function is called,
>> we lost track of the call chain, like which virtual device was it
>> originating from. If we somehow added a notification information about
>> the network device stack (and we could use netdevice notifiers for
>> that), then maybe we don't really need to add all of this code and we
>> can just derive the necessary bits of information we want by checking:
>> is this a VLAN network device? It is, okay what's your VLAN ID, etc.?
>>
>> Either approach would get us our cookie anyway :)
> 
> Postulate here is that address of vlan device is separate from netdevice
> entity
> with it's own context.
> 
> Several cases talking about this:
> 
> - bound device having 2 slaves can have added vid to both slave devices but
> synced addresses for only one of them. So, if vid is set in real device
> it doesn't mean it needs addresses of vlan device.
> 
> - I know that's crazy, but net device tree can contain 2 same vlan
> devices ).
> The scheme doesn't prevent this case. So one vid address can be counted
> by two
> vlan network devices.
> 
> - Any of the devices in _sync/rx_mode() chain is legal to do with addresses
> what ever it's allowed to do, drop some of them, combine with others and
> more,
> even add it's own vid addresses w/o actual vlan network device.
> 
> These made me to look in side of building rx_sync netdevice tree holding
> links
> on nodes per each address. And I've did this mostly...then after short look
> on this asked myself "who is going to support this ..." and it anyway
> doesn't
> cover all possible cases.
> 
> So, lost track of the device looks not so bad and opens more possibilities.

Sounds like you did give a lot of thoughts about the scheme, I am fine
with the current approach and will hook it up to DSA next week to make
sure this resolves the current issues I had with VLAN filtering enabled.
Thanks Ivan!
diff mbox series

Patch

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 94657f3c483a..9c914b31d208 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -137,6 +137,7 @@  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 void vlan_dev_ivdf_set(struct net_device *dev, int enable);
 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/Kconfig b/net/8021q/Kconfig
index 42320180967f..3e843045739c 100644
--- a/net/8021q/Kconfig
+++ b/net/8021q/Kconfig
@@ -38,3 +38,15 @@  config VLAN_8021Q_MVRP
 	  supersedes GVRP and is not backwards-compatible.
 
 	  If unsure, say N.
+
+config VLAN_8021Q_IVDF
+	bool "IVDF (Individual Virtual Device Filtering) support"
+	depends on VLAN_8021Q
+	help
+	  Select this to enable IVDF addressing scheme support. IVDF is used
+	  for automatic propagation of registered VLANs addresses to real end
+	  devices. If no device supporting IVDF then disable this as it can
+	  consume some memory in configuration with complex network device
+	  structures to hold vlan addresses.
+
+	  If unsure, say N.
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index fe2ac64c13f8..310b6cd39f22 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -454,6 +454,18 @@  bool vlan_uses_dev(const struct net_device *dev)
 }
 EXPORT_SYMBOL(vlan_uses_dev);
 
+void vlan_dev_ivdf_set(struct net_device *dev, int enable)
+{
+#ifdef CONFIG_VLAN_8021Q_IVDF
+	if (enable) {
+		dev->vid_len = NET_8021Q_VID_TSIZE;
+		return;
+	}
+#endif
+	dev->vid_len = 0;
+}
+EXPORT_SYMBOL(vlan_dev_ivdf_set);
+
 u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8 *addr)
 {
 	u16 vid = 0;
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 634436e780f1..e4120aca4b9b 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -896,5 +896,6 @@  void vlan_setup(struct net_device *dev)
 	dev->min_mtu		= 0;
 	dev->max_mtu		= ETH_MAX_MTU;
 
+	vlan_dev_ivdf_set(dev, 0);
 	eth_zero_addr(dev->broadcast);
 }
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index f7a3d7a171c7..95497cac24eb 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -381,6 +381,7 @@  void ether_setup(struct net_device *dev)
 	dev->flags		= IFF_BROADCAST|IFF_MULTICAST;
 	dev->priv_flags		|= IFF_TX_SKB_SHARING;
 
+	vlan_dev_ivdf_set(dev, 1);
 	eth_broadcast_addr(dev->broadcast);
 
 }
@@ -404,8 +405,13 @@  EXPORT_SYMBOL(ether_setup);
 struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
 				      unsigned int rxqs)
 {
-	return alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_UNKNOWN,
-				ether_setup, txqs, rxqs);
+	struct net_device *dev;
+
+	dev = alloc_netdev_mqs(sizeof_priv, "eth%d", NET_NAME_UNKNOWN,
+			       ether_setup, txqs, rxqs);
+
+	vlan_dev_ivdf_set(dev, 0);
+	return dev;
 }
 EXPORT_SYMBOL(alloc_etherdev_mqs);