| Message ID | 1520970647-19587-1-git-send-email-s.mesoraca16@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: drop some VLAs in switch.c | expand |
Hi Salvatore, Salvatore Mesoraca <s.mesoraca16@gmail.com> writes: > 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 <s.mesoraca16@gmail.com> 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. Thanks, Vivien
On 03/13/2018 12:58 PM, Vivien Didelot wrote: > Hi Salvatore, > > Salvatore Mesoraca <s.mesoraca16@gmail.com> writes: > >> 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 <s.mesoraca16@gmail.com> > > 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.
2018-03-13 20:58 GMT+01:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>: > Hi Salvatore, Hi Vivien, > Salvatore Mesoraca <s.mesoraca16@gmail.com> writes: > >> 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 <s.mesoraca16@gmail.com> > > 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
From: Salvatore Mesoraca > Sent: 13 March 2018 22:01 > 2018-03-13 20:58 GMT+01:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>: > > Hi Salvatore, > > Hi Vivien, > > > Salvatore Mesoraca <s.mesoraca16@gmail.com> writes: > > > >> 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 <s.mesoraca16@gmail.com> > > > > 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. Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less than the number of bits in a word? David
2018-03-14 12:24 GMT+01:00 David Laight <David.Laight@aculab.com>: > 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.
2018-03-14 13:48 GMT+01:00 Salvatore Mesoraca <s.mesoraca16@gmail.com>: > 2018-03-14 12:24 GMT+01:00 David Laight <David.Laight@aculab.com>: >> 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
2018-03-13 21:06 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>: > On 03/13/2018 12:58 PM, Vivien Didelot wrote: >> Hi Salvatore, >> >> Salvatore Mesoraca <s.mesoraca16@gmail.com> writes: >> >>> 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 <s.mesoraca16@gmail.com> >> >> 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. Hi Florian, Should I consider this patch still NAKed or not? Should I resend the patch with some modifications? Thank you, Salvatore
On Sat, May 05, 2018 at 12:36:36PM +0200, Salvatore Mesoraca wrote: > 2018-03-13 21:06 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>: > > On 03/13/2018 12:58 PM, Vivien Didelot wrote: > >> Hi Salvatore, > >> > >> Salvatore Mesoraca <s.mesoraca16@gmail.com> writes: > >> > >>> 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 <s.mesoraca16@gmail.com> > >> > >> 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. > > 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
On Sat, May 5, 2018 at 8:39 AM, Andrew Lunn <andrew@lunn.ch> wrote: > On Sat, May 05, 2018 at 12:36:36PM +0200, Salvatore Mesoraca wrote: >> 2018-03-13 21:06 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>: >> > On 03/13/2018 12:58 PM, Vivien Didelot wrote: >> >> Hi Salvatore, >> >> >> >> Salvatore Mesoraca <s.mesoraca16@gmail.com> writes: >> >> >> >>> 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 <s.mesoraca16@gmail.com> >> >> >> >> 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. >> >> 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(). Are dsa_switch_mdb_add() and dsa_switch_vlan_add() guaranteed to be single-threaded? -Kees
> > 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
diff --git a/net/dsa/switch.c b/net/dsa/switch.c index b935117..78e9897 100644 --- a/net/dsa/switch.c +++ b/net/dsa/switch.c @@ -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 */
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 <s.mesoraca16@gmail.com> --- net/dsa/switch.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)