Subject: Re: [PATCH 5/6] bridge: new function to pack vlans using both IFLA_BRIDGE_VLAN_INFO and IFLA_BRIDGE_VLAN_RANGE_INFO

From: roopa <[email-masked]>
Date: Wed, 31 Dec 2014 17:50:56 -0800

On 12/29/14, 9:31 PM, roopa wrote:
> On 12/29/14, 3:25 PM, Scott Feldman wrote:
>> On Mon, Dec 29, 2014 at 1:05 PM, <[email-masked]> wrote:
>>> From: Roopa Prabhu <[email-masked]>
>>>
>>> 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 <[email-masked]>
>>> Signed-off-by: Roopa Prabhu <[email-masked]>
>>> ---
>>>   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
>
>>> +       u16 pvid;
>> Aren't you getting an "unused" compile warning on this ^^^?
> will double check..
>
>>> +
>>> +       /* handle the untagged */
>> This comment ^^^ doesn't make sense here.
>>
>>> +       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...
>
>>> +       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.
>>
>>> +               }
>>> +       }
>>> +
>>> +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 ?
>
>>
>>> +       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.
>>   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.
>
>>> +
>>> +       /* 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.
>
>>> +       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 [email-masked]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>