On 12/29/14, 9:31 PM, roopa 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 ?

quoted

quoted

+       struct bridge_vlan_range_info vinfo_range;
+       struct bridge_vlan_info vinfo;
+       u16 pvid;
+
+       memset(vlan_bmp_copy, 0,
+              sizeof(unsigned long) * BR_VLAN_BITMAP_LEN);
+       bitmap_copy(vlan_bmp_copy, pv->vlan_bitmap, VLAN_N_VID);
+
+       memset(untagged_bmp_copy, 0,
+              sizeof(unsigned long) * BR_VLAN_BITMAP_LEN);
+       bitmap_copy(untagged_bmp_copy, pv->untagged_bitmap, 
VLAN_N_VID);
+
+       /* send the pvid separately first */
+       pvid = br_get_pvid(pv);
+       if (pvid && (pvid != VLAN_N_VID)) {
+               memset(&vinfo, 0, sizeof(vinfo));
+               vinfo.flags |= BRIDGE_VLAN_INFO_PVID;
+               if (test_bit(pvid, untagged_bmp_copy)) {
+                       vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED;
+                       clear_bit(pvid, untagged_bmp_copy);
+               }
+               clear_bit(pvid, vlan_bmp_copy);
+               vinfo.vid = pvid;
+               if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
+                           sizeof(vinfo), &vinfo))
+                       goto nla_put_failure;
+       }

What if pvid is not in vlan_bmp_copy?  This statement block seems
slightly different than the original logic where BRIDGE_VLAN_INFO_PVID
was set only when looping thru vlan_bitmap and vid == pvid.  Now can
you send a IFLA_BRIDGE_VLAN_INFO with pvid even if pvid isn't in
vlan_bitmap?

yes, you can AFAIK. pvid is usually untagged.

quoted

  Maybe pvid is always in vlan_bitmap, so it doesn't
matter.  But there is a subtle logic change here, so something to
consider.

Understand what you are saying, I will check this with v2.

quoted

quoted

+
+       /* fill untagged vlans */
+       br_fill_ifvlaninfo_bitmap(skb, untagged_bmp_copy,
+                                 BRIDGE_VLAN_INFO_UNTAGGED);

This might be doing more than original logic.  Original logic looped
thru vid in vlan_btimap and checked if vid was in untagged_bitmap.
Here you're dumping all bits in untagged_bitmap, without looking if
bit was set in vlan_bitmap.  Maybe you want to do an intersection of
sets vlan_bitmap and untagged_bitmap.

There is a clear_bit for vlan_bmp_copy  below.
And all this works as expected on our boxes (Its tested code).

But, understand the difference with the original loop. Will confirm.

re-checked and the code is correct and does not deviate from original 
logic.
The vlan_bitmap has all vids  set in the untagged_bitmap.

plus, we will see if we can use bitmap intersection here.

quoted

quoted

+       for_each_set_bit(vid, untagged_bmp_copy, VLAN_N_VID)
+               clear_bit(vid, vlan_bmp_copy);
+
+       /* fill tagged vlans */
+       br_fill_ifvlaninfo_bitmap(skb, vlan_bmp_copy, 0);
+
+       return 0;
+
+nla_put_failure:
+       return -EMSGSIZE;
+}
+
+static int br_fill_ifvlaninfo(struct sk_buff *skb,
+                             const struct net_port_vlans *pv)
+{
+       struct bridge_vlan_info vinfo;
+       u16 pvid, vid;
+
+       pvid = br_get_pvid(pv);
+       for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
+               vinfo.vid = vid;
+               vinfo.flags = 0;
+               if (vid == pvid)
+                       vinfo.flags |= BRIDGE_VLAN_INFO_PVID;
+
+               if (test_bit(vid, pv->untagged_bitmap))
+                       vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED;
+
+               if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
+                           sizeof(vinfo), &vinfo))
+                       goto nla_put_failure;
+       }
+
+       return 0;
+
+nla_put_failure:
+       return -EMSGSIZE;
+}
+
  /*
   * Create one netlink message for one interface
   * Contains port and master info as well as carrier and bridge state.

@@ -121,12 +248,11 @@ static int br_fill_ifinfo(struct sk_buff *skb,
         }

         /* Check if  the VID information is requested */
-       if (filter_mask & RTEXT_FILTER_BRVLAN) {
-               struct nlattr *af;
+       if ((filter_mask & RTEXT_FILTER_BRVLAN) ||
+           (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)) {
                 const struct net_port_vlans *pv;
-               struct bridge_vlan_info vinfo;
-               u16 vid;
-               u16 pvid;
+               struct nlattr *af;
+               int err;

                 if (port)
                         pv = nbp_get_vlan_info(port);

@@ -140,21 +266,12 @@ static int br_fill_ifinfo(struct sk_buff *skb,
                 if (!af)
                         goto nla_put_failure;

-               pvid = br_get_pvid(pv);
-               for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
-                       vinfo.vid = vid;
-                       vinfo.flags = 0;
-                       if (vid == pvid)
-                               vinfo.flags |= BRIDGE_VLAN_INFO_PVID;
-
-                       if (test_bit(vid, pv->untagged_bitmap))
-                               vinfo.flags |= 

BRIDGE_VLAN_INFO_UNTAGGED;
-
-                       if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
-                                   sizeof(vinfo), &vinfo))
-                               goto nla_put_failure;
-               }
-
+               if (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)
+                       err = br_fill_ifvlaninfo_compressed(skb, pv);
+               else
+                       err = br_fill_ifvlaninfo(skb, pv);
+               if (err)
+                       goto nla_put_failure;
                 nla_nest_end(skb, af);
         }

-- 
1.7.10.4

-- 
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html