On Mon, Dec 29, 2014 at 9:31 PM, roopa [off-list ref] wrote:

On 12/29/14, 3:25 PM, Scott Feldman wrote:

quoted

On Mon, Dec 29, 2014 at 1:05 PM,  [off-list ref] wrote:

quoted

From: Roopa Prabhu <redacted>

This patch adds new function to compress vlans into ranges.
Vlans are compressed into ranges only if the fill request is called with
RTEXT_FILTER_BRVLAN_COMPRESSED in filtermask.

Old vlan packing code is moved to a new function and continues to be
called when filter_mask is RTEXT_FILTER_BRVLAN

Signed-off-by: Wilson kok <redacted>
Signed-off-by: Roopa Prabhu <redacted>
---
  net/bridge/br_netlink.c |  157
+++++++++++++++++++++++++++++++++++++++++------
  1 file changed, 137 insertions(+), 20 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 4c47ba0..16bdd5a 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c

@@ -67,6 +67,133 @@ static int br_port_fill_attrs(struct sk_buff *skb,
         return 0;
  }

+static int br_fill_ifvlaninfo_bitmap(struct sk_buff *skb,
+                                    const unsigned long *vlan_bmp, u16

flags)
+{
+       struct bridge_vlan_range_info vinfo_range;
+       struct bridge_vlan_info vinfo;
+       u16 vid, start = 0, end = 0;

One per line?

ack

quoted

quoted

+       u16 pvid;

Aren't you getting an "unused" compile warning on this ^^^?

will double check..

quoted

quoted

+
+       /* handle the untagged */

This comment ^^^ doesn't make sense here.

quoted

+       for_each_set_bit(vid, vlan_bmp, VLAN_N_VID) {
+               if (start == 0) {
+                       start = vid;
+                       end = vid;
+               }
+               if ((vid - end) > 1) {
+                       memset(&vinfo_range, 0, sizeof(vinfo_range));
+                       vinfo_range.flags |= flags;
+                       vinfo_range.vid = start;
+                       vinfo_range.vid_end = end;
+                       if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO,
+                                   sizeof(vinfo_range), &vinfo_range))
+                               goto nla_put_failure;
+                       start = vid;
+                       end = vid;
+               } else {
+                       end = vid;
+               }
+       }

What happens with this set {1-10, 12, 20-25, 30}?  On vid 12, will
that put a VLAN_RANGE_INFO with start=end=12?  Seems strange to use
RANGE_INFO for single vlan.

Can the algorithm be simplified?  Maybe there are other examples in
the kernel of finding ranges/singles from a bitmap we could borrow
code from?

let me see...

quoted

quoted

+       if (start != 0 && end != 0) {
+               if (start != end) {
+                       memset(&vinfo_range, 0, sizeof(vinfo_range));
+                       vinfo_range.flags |= flags;
+                       vinfo_range.vid = start;
+                       vinfo_range.vid_end = end;
+                       if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO,
+                                   sizeof(vinfo_range), &vinfo_range))
+                               goto nla_put_failure;
+               } else {
+                       memset(&vinfo, 0, sizeof(vinfo));
+                       vinfo.flags |= flags;
+                       vinfo.vid = start;
+                       if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
+                                   sizeof(vinfo), &vinfo))
+                               goto nla_put_failure;

How many IFLA_BRIDGE_VLAN_INFOs can you fit into skb?   It seems the
worst case is you'll have 4094/2 = 2047 VLAN_INFOs if using all
even-numbered vids, for example.  Will that fit?  I guess the original
code had the same concern...wonder if anyone checked this worst-case?

I will check this again. Remember doing worst case tests for setlinks. will
get back with some worst cases tests in the next submission.

quoted

quoted

+               }
+       }
+
+nla_put_failure:
+       return -EMSGSIZE;
+}
+
+static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb,
+                                        const struct net_port_vlans *pv)
+{
+       unsigned long vlan_bmp_copy[BR_VLAN_BITMAP_LEN];
+       unsigned long untagged_bmp_copy[BR_VLAN_BITMAP_LEN];

Lots of automatic space on the stack...can you use dynamic mem
(kzalloc) for these?

Let me see. Or make it a static global ?

Static global would be bad news for multiple threads doing a vlan fill.