Re: [PATCH v2] net: dsa: drop some VLAs in switch.c

[Salvatore Mesoraca]

2018-05-08 11:39 GMT+02:00 David Laight <david.lai...@aculab.com>:
> From: Salvatore Mesoraca
>> Sent: 07 May 2018 20:03
> ...
>> This optimization will save us an allocation when number of ports is
>> less than 32 or 64 (depending on arch).
>> IMHO it's useful, if you consider that, right now, DSA works only with
>> 12-ports switches.
>
> Why not just error out if the number of ports is greater than the compile-time
> limit?
>
> Worry about dynamic allocation if you need a lot more than 64 ports.

v1 has been NAK-ed by maintainers because they don't want limits on how
many ports a switch can have.

Salvatore

Re: [PATCH v2] net: dsa: drop some VLAs in switch.c

[Salvatore Mesoraca]

2018-05-07 21:26 GMT+02:00 Andrew Lunn :
>> >> +++ b/include/net/dsa.h
>> >> @@ -256,6 +256,9 @@ struct dsa_switch {
>> >>   /* Number of switch port queues */
>> >>   unsigned intnum_tx_queues;
>> >>
>> >> + unsigned long   *bitmap;
>> >> + unsigned long   _bitmap;
>> >> +
>> >>   /* Dynamically allocated ports, keep last */
>> >>   size_t num_ports;
>> >>   struct dsa_port ports[];
>> >> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
>> >> index adf50fb..cebf35f0 100644
>> >> --- a/net/dsa/dsa2.c
>> >> +++ b/net/dsa/dsa2.c
>> >> @@ -748,6 +748,20 @@ struct dsa_switch *dsa_switch_alloc(struct device 
>> >> *dev, size_t n)
>> >>   if (!ds)
>> >>   return NULL;
>> >>
>> >> + /* We avoid allocating memory outside dsa_switch
>> >> +  * if it is not needed.
>> >> +  */
>> >> + if (n <= sizeof(ds->_bitmap) * 8) {
>> >> + ds->bitmap = >_bitmap;
>> >
>> > Should not this be / BITS_PER_BYTE? If the sizeof(unsigned long) is <=
>> > 8, then you don't need to allocate it, otherwise, you have to.
>
>> This optimization will save us an allocation when number of ports is
>> less than 32 or 64 (depending on arch).
>> IMHO it's useful, if you consider that, right now, DSA works only with
>> 12-ports switches.
>
> Do you have a feeling for the savings? I don't see it being very
> large, and given the extra code, it might actually be negative.

I think that a "compare" and a "jump" costs nothing compared to
devm_kmalloc, its eventual free, and, *maybe*, the cache miss you
get every time you access the bitmask.
This is not necessarily relevant if this code it's invoked rarely,
but, IMHO, it seems strange to always go for dynamic allocation for
something that will be, almost always, as big as a pointer.

Salvatore

Re: [PATCH v2] net: dsa: drop some VLAs in switch.c

[Salvatore Mesoraca]

2018-05-07 20:14 GMT+02:00 Florian Fainelli <f.faine...@gmail.com>:
> On 05/07/2018 08:23 AM, Salvatore Mesoraca wrote:
>> We avoid 2 VLAs by using a pre-allocated field in dsa_switch.
>> We also try to avoid dynamic allocation whenever possible.
>>
>> Link: 
>> http://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>> Link: http://lkml.kernel.org/r/20180505185145.gb32...@lunn.ch
>>
>> Signed-off-by: Salvatore Mesoraca <s.mesorac...@gmail.com>
>> ---
>>  include/net/dsa.h |  3 +++
>>  net/dsa/dsa2.c| 14 ++
>>  net/dsa/switch.c  | 22 ++
>>  3 files changed, 27 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 60fb4ec..576791d 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -256,6 +256,9 @@ struct dsa_switch {
>>   /* Number of switch port queues */
>>   unsigned intnum_tx_queues;
>>
>> + unsigned long   *bitmap;
>> + unsigned long   _bitmap;
>> +
>>   /* Dynamically allocated ports, keep last */
>>   size_t num_ports;
>>   struct dsa_port ports[];
>> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
>> index adf50fb..cebf35f0 100644
>> --- a/net/dsa/dsa2.c
>> +++ b/net/dsa/dsa2.c
>> @@ -748,6 +748,20 @@ struct dsa_switch *dsa_switch_alloc(struct device *dev, 
>> size_t n)
>>   if (!ds)
>>   return NULL;
>>
>> + /* We avoid allocating memory outside dsa_switch
>> +  * if it is not needed.
>> +  */
>> + if (n <= sizeof(ds->_bitmap) * 8) {
>> + ds->bitmap = >_bitmap;
>
> Should not this be / BITS_PER_BYTE? If the sizeof(unsigned long) is <=
> 8, then you don't need to allocate it, otherwise, you have to.

No.
We need one 1 bit per port, of course sizeof() returns size in bytes,
hence the multiplication to get the number of bits.
I might multiply per BITS_PER_BYTE instead of 8, but I doubt that
Linux supports implementations where a byte is not an octet.

> I would actually just always dynamically allocate the bitmap, optimizing
> for the case where we have fewer than or 8 ports is not worth IMHO.

This optimization will save us an allocation when number of ports is
less than 32 or 64 (depending on arch).
IMHO it's useful, if you consider that, right now, DSA works only with
12-ports switches.

Thank you for your time,

Salvatore

[PATCH v2] net: dsa: drop some VLAs in switch.c

[Salvatore Mesoraca]

We avoid 2 VLAs by using a pre-allocated field in dsa_switch.
We also try to avoid dynamic allocation whenever possible.

Link: 
http://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
Link: http://lkml.kernel.org/r/20180505185145.gb32...@lunn.ch

Signed-off-by: Salvatore Mesoraca <s.mesorac...@gmail.com>
---
 include/net/dsa.h |  3 +++
 net/dsa/dsa2.c| 14 ++
 net/dsa/switch.c  | 22 ++
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 60fb4ec..576791d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -256,6 +256,9 @@ struct dsa_switch {
/* Number of switch port queues */
unsigned intnum_tx_queues;
 
+   unsigned long   *bitmap;
+   unsigned long   _bitmap;
+
/* Dynamically allocated ports, keep last */
size_t num_ports;
struct dsa_port ports[];
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index adf50fb..cebf35f0 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -748,6 +748,20 @@ struct dsa_switch *dsa_switch_alloc(struct device *dev, 
size_t n)
if (!ds)
return NULL;
 
+   /* We avoid allocating memory outside dsa_switch
+* if it is not needed.
+*/
+   if (n <= sizeof(ds->_bitmap) * 8) {
+   ds->bitmap = >_bitmap;
+   } else {
+   ds->bitmap = devm_kzalloc(dev,
+ BITS_TO_LONGS(n) *
+   sizeof(unsigned long),
+ GFP_KERNEL);
+   if (unlikely(!ds->bitmap))
+   return NULL;
+   }
+
ds->dev = dev;
ds->num_ports = n;
 
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index b935117..142b294 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -136,21 +136,20 @@ 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);
int port;
 
/* Build a mask of Multicast group members */
-   bitmap_zero(group, ds->num_ports);
+   bitmap_zero(ds->bitmap, ds->num_ports);
if (ds->index == info->sw_index)
-   set_bit(info->port, group);
+   set_bit(info->port, ds->bitmap);
for (port = 0; port < ds->num_ports; port++)
if (dsa_is_dsa_port(ds, port))
-   set_bit(port, group);
+   set_bit(port, ds->bitmap);
 
if (switchdev_trans_ph_prepare(trans))
-   return dsa_switch_mdb_prepare_bitmap(ds, mdb, group);
+   return dsa_switch_mdb_prepare_bitmap(ds, mdb, ds->bitmap);
 
-   dsa_switch_mdb_add_bitmap(ds, mdb, group);
+   dsa_switch_mdb_add_bitmap(ds, mdb, ds->bitmap);
 
return 0;
 }
@@ -204,21 +203,20 @@ 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);
int port;
 
/* Build a mask of VLAN members */
-   bitmap_zero(members, ds->num_ports);
+   bitmap_zero(ds->bitmap, ds->num_ports);
if (ds->index == info->sw_index)
-   set_bit(info->port, members);
+   set_bit(info->port, ds->bitmap);
for (port = 0; port < ds->num_ports; port++)
if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
-   set_bit(port, members);
+   set_bit(port, ds->bitmap);
 
if (switchdev_trans_ph_prepare(trans))
-   return dsa_switch_vlan_prepare_bitmap(ds, vlan, members);
+   return dsa_switch_vlan_prepare_bitmap(ds, vlan, ds->bitmap);
 
-   dsa_switch_vlan_add_bitmap(ds, vlan, members);
+   dsa_switch_vlan_add_bitmap(ds, vlan, ds->bitmap);
 
return 0;
 }
-- 
1.9.1

Re: [PATCH] net: dsa: drop some VLAs in switch.c

[Salvatore Mesoraca]

2018-03-13 21:06 GMT+01:00 Florian Fainelli <f.faine...@gmail.com>:
> On 03/13/2018 12:58 PM, Vivien Didelot wrote:
>> Hi Salvatore,
>>
>> Salvatore Mesoraca <s.mesorac...@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.mesorac...@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

Re: [PATCH] net: dsa: drop some VLAs in switch.c

[Salvatore Mesoraca]

2018-03-14 13:48 GMT+01:00 Salvatore Mesoraca <s.mesorac...@gmail.com>:
> 2018-03-14 12:24 GMT+01:00 David Laight <david.lai...@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

Re: [PATCH] net: dsa: drop some VLAs in switch.c

[Salvatore Mesoraca]

2018-03-14 12:24 GMT+01:00 David Laight :
> 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.

Re: [PATCH] net: dsa: drop some VLAs in switch.c

[Salvatore Mesoraca]

2018-03-13 20:58 GMT+01:00 Vivien Didelot <vivien.dide...@savoirfairelinux.com>:
> Hi Salvatore,

Hi Vivien,

> Salvatore Mesoraca <s.mesorac...@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.mesorac...@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

[PATCH] net: dsa: drop some VLAs in switch.c

[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 <s.mesorac...@gmail.com>
---
 net/dsa/switch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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 */
-- 
1.9.1

Re: [PATCH] net: llc: drop VLA in llc_sap_mcast()

[Salvatore Mesoraca]

2018-03-12 16:14 GMT+01:00 David Miller <da...@davemloft.net>:
> From: Salvatore Mesoraca <s.mesorac...@gmail.com>
> Date: Sun, 11 Mar 2018 22:12:04 +0100
>
>> Avoid a VLA[1] by using a real constant expression instead of a variable.
>> The compiler should be able to optimize the original code and avoid using
>> an actual VLA. Anyway this change is useful because it will avoid a false
>> positive with -Wvla, it might also help the compiler generating better
>> code.
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Salvatore Mesoraca <s.mesorac...@gmail.com>
>
> Applied.

Thank you.

Re: [PATCH 1/2] net: rds: drop VLA in rds_for_each_conn_info()

[Salvatore Mesoraca]

2018-03-12 8:06 GMT+01:00 santosh.shilim...@oracle.com
<santosh.shilim...@oracle.com>:
> On 3/11/18 2:07 PM, Salvatore Mesoraca wrote:
>>
>> Avoid VLA[1] by using an already allocated buffer passed
>> by the caller.
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Salvatore Mesoraca <s.mesorac...@gmail.com>
>> ---
>
> Thanks for both VLA fixes Salvatore.
>
> FWIW, Acked-by: Santosh Shilimkar <santosh.shilim...@oracle.com>

Thank you very much for your time,

Salvatore

[PATCH] net: llc: drop VLA in llc_sap_mcast()

[Salvatore Mesoraca]

Avoid a VLA[1] by using a real constant expression instead of a variable.
The compiler should be able to optimize the original code and avoid using
an actual VLA. Anyway this change is useful because it will avoid a false
positive with -Wvla, it might also help the compiler generating better
code.

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

Signed-off-by: Salvatore Mesoraca <s.mesorac...@gmail.com>
---
 net/llc/llc_sap.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/llc/llc_sap.c b/net/llc/llc_sap.c
index d90928f..a7f7b8f 100644
--- a/net/llc/llc_sap.c
+++ b/net/llc/llc_sap.c
@@ -394,8 +394,9 @@ static void llc_sap_mcast(struct llc_sap *sap,
  const struct llc_addr *laddr,
  struct sk_buff *skb)
 {
-   int i = 0, count = 256 / sizeof(struct sock *);
-   struct sock *sk, *stack[count];
+   int i = 0;
+   struct sock *sk;
+   struct sock *stack[256 / sizeof(struct sock *)];
struct llc_sock *llc;
struct hlist_head *dev_hb = llc_sk_dev_hash(sap, skb->dev->ifindex);
 
@@ -408,7 +409,7 @@ static void llc_sap_mcast(struct llc_sap *sap,
continue;
 
sock_hold(sk);
-   if (i < count)
+   if (i < ARRAY_SIZE(stack))
stack[i++] = sk;
else {
llc_do_mcast(sap, skb, stack, i);
-- 
1.9.1

[PATCH 2/2] net: rds: drop VLA in rds_walk_conn_path_info()

[Salvatore Mesoraca]

Avoid VLA[1] by using an already allocated buffer passed
by the caller.

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

Signed-off-by: Salvatore Mesoraca <s.mesorac...@gmail.com>
---
 net/rds/connection.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index f80792c..abef75d 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -578,9 +578,9 @@ static void rds_walk_conn_path_info(struct socket *sock, 
unsigned int len,
struct rds_info_iterator *iter,
struct rds_info_lengths *lens,
int (*visitor)(struct rds_conn_path *, void 
*),
+   u64 *buffer,
size_t item_len)
 {
-   u64  buffer[(item_len + 7) / 8];
struct hlist_head *head;
struct rds_connection *conn;
size_t i;
@@ -649,8 +649,11 @@ static void rds_conn_info(struct socket *sock, unsigned 
int len,
  struct rds_info_iterator *iter,
  struct rds_info_lengths *lens)
 {
+   u64 buffer[(sizeof(struct rds_info_connection) + 7) / 8];
+
rds_walk_conn_path_info(sock, len, iter, lens,
rds_conn_info_visitor,
+   buffer,
sizeof(struct rds_info_connection));
 }
 
-- 
1.9.1

[PATCH 1/2] net: rds: drop VLA in rds_for_each_conn_info()

[Salvatore Mesoraca]

Avoid VLA[1] by using an already allocated buffer passed
by the caller.

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

Signed-off-by: Salvatore Mesoraca <s.mesorac...@gmail.com>
---
 net/rds/connection.c | 2 +-
 net/rds/ib.c | 3 +++
 net/rds/rds.h| 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 2da3176..f80792c 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -540,9 +540,9 @@ void rds_for_each_conn_info(struct socket *sock, unsigned 
int len,
  struct rds_info_iterator *iter,
  struct rds_info_lengths *lens,
  int (*visitor)(struct rds_connection *, void *),
+ u64 *buffer,
  size_t item_len)
 {
-   uint64_t buffer[(item_len + 7) / 8];
struct hlist_head *head;
struct rds_connection *conn;
size_t i;
diff --git a/net/rds/ib.c b/net/rds/ib.c
index 50a88f3..02deee2 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -321,8 +321,11 @@ static void rds_ib_ic_info(struct socket *sock, unsigned 
int len,
   struct rds_info_iterator *iter,
   struct rds_info_lengths *lens)
 {
+   u64 buffer[(sizeof(struct rds_info_rdma_connection) + 7) / 8];
+
rds_for_each_conn_info(sock, len, iter, lens,
rds_ib_conn_info_visitor,
+   buffer,
sizeof(struct rds_info_rdma_connection));
 }
 
diff --git a/net/rds/rds.h b/net/rds/rds.h
index 7301b9b..91ea08f 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -709,6 +709,7 @@ void rds_for_each_conn_info(struct socket *sock, unsigned 
int len,
  struct rds_info_iterator *iter,
  struct rds_info_lengths *lens,
  int (*visitor)(struct rds_connection *, void *),
+ u64 *buffer,
  size_t item_len);
 
 __printf(2, 3)
-- 
1.9.1