Skip to contentSkip to conversation

net: dsa: drop some VLAs in switch.c

PATCH Superseded Salvatore Mesoraca 6 participants 11 messages Mar 13, 2018 – May 5, 2018

Subsystems (3)
Networking Network drivers Networking DSA

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Salvatore Mesoraca

author
#0 lore raw

PATCH 1/1 net: dsa: drop some VLAs in switch.c

dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by Salvatore Mesoraca <[email protected]>
---
 net/dsa/switch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/dsa/switch.c +2 -2
index b935117..78e9897 100644
@@ -136,7 +136,7 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
 {
 	const struct switchdev_obj_port_mdb *mdb = info->mdb;
 	struct switchdev_trans *trans = info->trans;
-	DECLARE_BITMAP(group, ds->num_ports);
+	DECLARE_BITMAP(group, DSA_MAX_PORTS);
 	int port;
 
 	/* Build a mask of Multicast group members */

@@ -204,7 +204,7 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
 {
 	const struct switchdev_obj_port_vlan *vlan = info->vlan;
 	struct switchdev_trans *trans = info->trans;
-	DECLARE_BITMAP(members, ds->num_ports);
+	DECLARE_BITMAP(members, DSA_MAX_PORTS);
 	int port;
 
 	/* Build a mask of VLAN members */
Signature · 1 line
1.9.1
#2 lore raw
Quoted 7 lines · Vivien Didelot

Hi Salvatore,

Quoted 3 lines · Salvatore Mesoraca

dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.

Quoted 8 lines
Quoted 3 lines

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by Salvatore Mesoraca <[email protected]>

NAK.

We are in the process to remove hardcoded limits such as DSA_MAX_PORTS and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.

Then this means that we need to allocate a bitmap from the heap, which sounds a bit superfluous and could theoretically fail... not sure which way is better, but bumping the size to DSA_MAX_PORTS definitively does help people working on enabling -Wvla.

Signature · 1 line
Florian
author
#3 lore raw

2018-03-13 21:06 GMT+01:00 Florian Fainelli <[email protected]>:

Quoted 14 lines · Vivien Didelot
Quoted 13 lines

Hi Salvatore,

Quoted 6 lines · Salvatore Mesoraca

dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by Salvatore Mesoraca <[email protected]>

NAK.

Quoted 7 lines
Quoted 2 lines

We are in the process to remove hardcoded limits such as DSA_MAX_PORTS and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.

Then this means that we need to allocate a bitmap from the heap, which sounds a bit superfluous and could theoretically fail... not sure which way is better, but bumping the size to DSA_MAX_PORTS definitively does help people working on enabling -Wvla.

Hi Florian,

Should I consider this patch still NAKed or not? Should I resend the patch with some modifications?

Thank you,

Salvatore

maintainer
#4 lore raw
Quoted 23 lines · Salvatore Mesoraca

2018-03-13 21:06 GMT+01:00 Florian Fainelli <[email protected]>:

Quoted 14 lines · Vivien Didelot
Quoted 13 lines

Hi Salvatore,

Quoted 6 lines · Salvatore Mesoraca

dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by Salvatore Mesoraca <[email protected]>

NAK.

Quoted 7 lines
Quoted 2 lines

We are in the process to remove hardcoded limits such as DSA_MAX_PORTS and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.

Then this means that we need to allocate a bitmap from the heap, which sounds a bit superfluous and could theoretically fail... not sure which way is better, but bumping the size to DSA_MAX_PORTS definitively does help people working on enabling -Wvla.

Quoted 4 lines

Hi Florian,

Should I consider this patch still NAKed or not? Should I resend the patch with some modifications?

Hi Salvatore

We have been removing all uses of DSA_MAX_PORTS. I don't particularly like arbitrary limits on how many ports a switch can have, or how many switches a board can have.

So i would prefer to not use DSA_MAX_PORTS here.

You could make the bitmap part of the dsa_switch structure. This is allocated by dsa_switch_alloc() and is passed the number of ports. Doing the allocation there means you don't need to worry about it failing in dsa_switch_mdb_add() or dsa_switch_vlan_add().

Andrew

#5 lore raw
Quoted 35 lines · Andrew Lunn
Quoted 23 lines · Salvatore Mesoraca

2018-03-13 21:06 GMT+01:00 Florian Fainelli <[email protected]>:

Quoted 14 lines · Vivien Didelot
Quoted 13 lines

Hi Salvatore,

Quoted 6 lines · Salvatore Mesoraca

dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by Salvatore Mesoraca <[email protected]>

NAK.

Quoted 7 lines
Quoted 2 lines

We are in the process to remove hardcoded limits such as DSA_MAX_PORTS and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.

Then this means that we need to allocate a bitmap from the heap, which sounds a bit superfluous and could theoretically fail... not sure which way is better, but bumping the size to DSA_MAX_PORTS definitively does help people working on enabling -Wvla.

Quoted 4 lines

Hi Florian,

Should I consider this patch still NAKed or not? Should I resend the patch with some modifications?

Hi Salvatore

We have been removing all uses of DSA_MAX_PORTS. I don't particularly like arbitrary limits on how many ports a switch can have, or how many switches a board can have.

Quoted 6 lines

So i would prefer to not use DSA_MAX_PORTS here.

You could make the bitmap part of the dsa_switch structure. This is allocated by dsa_switch_alloc() and is passed the number of ports. Doing the allocation there means you don't need to worry about it failing in dsa_switch_mdb_add() or dsa_switch_vlan_add().

Are dsa_switch_mdb_add() and dsa_switch_vlan_add() guaranteed to be single-threaded?

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
maintainer
#6 lore raw
Quoted 7 lines
Quoted 4 lines

You could make the bitmap part of the dsa_switch structure. This is allocated by dsa_switch_alloc() and is passed the number of ports. Doing the allocation there means you don't need to worry about it failing in dsa_switch_mdb_add() or dsa_switch_vlan_add().

Are dsa_switch_mdb_add() and dsa_switch_vlan_add() guaranteed to be single-threaded?

Yes, that is the interesting question here.... against each other, or themselves?

They are called from a notifier chain. It is the same notifier chain for both dsa_switch_mdb_add() and dsa_switch_vlan_add().

notifier_call_chain() itself appears to not provide any guarantees about the same handler being called in parallel.

It is dsa_port_notify() which is calling the notifier_call_chain(). This is being called by both dsa_port_vlan_add() and dsa_port_mdb_add() in dsa_slave_port_obj_add(). This is a switchdev op. switchdev_port_obj_add_now() does have ASSERT_RTNL(); So that should serialize everything.

Andrew

author
#7 lore raw
In reply to Vivien Didelot

2018-03-13 20:58 GMT+01:00 Vivien Didelot <[email protected]>:

Quoted 1 line

Hi Salvatore,

Hi Vivien,

Quoted 5 lines
Quoted 3 lines · Salvatore Mesoraca

dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.

Quoted 8 lines
Quoted 3 lines

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by Salvatore Mesoraca <[email protected]>

NAK.

We are in the process to remove hardcoded limits such as DSA_MAX_PORTS and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.

I can rewrite the patch using kmalloc.
Although, if ds->num_ports will always be less than or equal to 12, it should be better to
just use DSA_MAX_PORTS.

Thank you,

Salvatore

#8 lore raw

From: Salvatore Mesoraca

Quoted 17 lines

Sent: 13 March 2018 22:01
2018-03-13 20:58 GMT+01:00 Vivien Didelot <[email protected]>:

Quoted 1 line

Hi Salvatore,

Hi Vivien,

Quoted 11 lines
Quoted 6 lines · Salvatore Mesoraca

dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by Salvatore Mesoraca <[email protected]>

NAK.

Quoted 7 lines
Quoted 2 lines

We are in the process to remove hardcoded limits such as DSA_MAX_PORTS and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.

I can rewrite the patch using kmalloc.
Although, if ds->num_ports will always be less than or equal to 12, it should be better to
just use DSA_MAX_PORTS.

Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less than the number of bits in a word?

David

author
#9 lore raw

2018-03-14 12:24 GMT+01:00 David Laight <[email protected]>:

Quoted 2 lines

Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less than the number of bits in a word?

It allocates ceiling(size/8) "unsigned long"s, so yes.

author
#10 lore raw

2018-03-14 13:48 GMT+01:00 Salvatore Mesoraca <[email protected]>:

Quoted 5 lines

2018-03-14 12:24 GMT+01:00 David Laight <[email protected]>:

Quoted 2 lines

Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less than the number of bits in a word?

It allocates ceiling(size/8) "unsigned long"s, so yes.

Actually I meant ceiling(size/8/sizeof(unsigned long)) I'm sorry for the typo.

Salvatore