Re: [PATCH v9 net-next 05/12] bridge: Add the ability to configure pvid

["Michał Mirosław" <mirqus@gmail.com>]

2013/2/3 Michał Mirosław <mirqus@gmail.com>:
> 2013/2/2 Vlad Yasevich <vyasevic@redhat.com>:
>> On 02/01/2013 08:15 PM, Michał Mirosław wrote:
>>> 2013/2/2 Michał Mirosław <mirqus@gmail.com>:
>>>> 2013/2/1 Vlad Yasevich <vyasevic@redhat.com>:
>>>>> A user may designate a certain vlan as PVID.  This means that
>>>>> any ingress frame that does not contain a vlan tag is assigned to
>>>>> this vlan and any forwarding decisions are made with this vlan in mind.
>>>>
>>>> [...]
>>>>>
>>>>>   struct net_port_vlans {
>>>>>          u16                             port_idx;
>>>>> +       u16                             pvid;
>>>>
>>>>
>>>> I'm confused about the implementation. I would expect pvid field in
>>>> net_bridge_port and adding a tag if it isn't there on ingress path.
>>>> The rest would be just like without PVIDs. But here you pvid field to
>>>> net_port_vlans, and don't do anything with it in receive nor transmit
>>>> path. Does it work? What am I missing?
>>> Found the answer in next patch (you should merge #5 and #6).
>> It was split for incremental testing.  #5 added the ability to set and
>> delete it without impacting anything.  #6 added the actual work that pvid
>> does.
>>
>>> Still,
>>> the implementation looks overly complicated. If you force the packet
>>> to canonical form on ingress (keeping outer tag in skb->vlan_tci, and
>>> setting skb->vlan_tci = pvid if there is no tag) the code should get
>>> simpler.
>>
>>
>> What if there is no outer tag?  That's what the ingress code is doing.
>> If there is no outer tag, pvid is written to vlan_tci.  If there was
>> outer tag in vlan_tci, it's left alone.  This way at the end of ingress
>> vlan_tci is always set.
>> At egress, we grab that tag and compare it against pvid if any.  If it
>> matches, it's stripped.  If it doesn't, we output with the tag thus
>> adding the header.
>>
>> The only thing I can simplify is grab the tci directly at egress, but
>> that's what the code will do anyway.
>
> ... br_allowed_input(..., struct sk_buff **pskb)
> {
>   *pskb = vlan_untag(*pskb);
>   skb = *pskb;
>   if (!skb)
>     return 0;
>
>   if (!skb->vlan_tci && (pvid & VLAN_TAG_PRESENT))
>     skb->vlan_tci = pvid;
>   return check_vlan_allowed(skb->vlan_tci);
> }

Or maybe some cheating to save unconditional vlan_untag()ging: since
vlan_tci is used only when vlan_tx_tag_present() returns true, we can
cache VID there for the bridge code when VLANs are not HW-accelerated.
I'm not sure of this one as I don't know if the netfilter part keeps
vlan_tci intact (quick grep suggests it does).

... br_allowed_input(..., struct sk_buff **pskb)
{
  skb = *pskb;
  if (!vlan_tx_tag_present(skb)) {
    if (!__vlan_get_tag(skb, &skb->vlan_tci))
      skb->vlan_tci &= VLAN_VID_MASK;
    else
      skb->vlan_tci = 0;
  }
  if (!skb->vlan_tci && pvid & VLAN_TAG_PRESENT)  // FIXME:
802.1p-tagged is put in VLAN 0
    skb->vlan_tci = pvid;
  return check_vlan_allowed(skb, skb->vlan_tci & VLAN_VID_MASK);
}

struct sk_buff *br_handle_vlan(..., struct sk_buff *skb)
{
   /* we guaranteed in br_allowed_input() that all packets processed
   /* in bridge code will be like received with NETIF_F_HW_VLAN_RX
    * feature enabled or have VID cached in vlan_tci without
    * VLAN_TAG_PRESENT bit set. */

   if ((skb->vlan_tci & VLAN_VID_MASK) | VLAN_TAG_PRESENT != pvid)
     return skb;

   if (!vlan_tx_tag_present(skb)) {
      skb = vlan_untag(skb);
      if (!skb)
        return skb;
   }
   skb->vlan_tci = 0;  // FIXME: what about 802.1p?
   return skb;
}

Best Regards,
Michał Mirosław