On 8/20/19 2:59 AM, Vladimir Oltean wrote: > Currently this simplified code snippet fails: > > br_vlan_get_pvid(netdev, &pvid); > br_vlan_get_info(netdev, pvid, &vinfo); > ASSERT(!(vinfo.flags & BRIDGE_VLAN_INFO_PVID)); > > It is intuitive that the pvid of a netdevice should have the > BRIDGE_VLAN_INFO_PVID flag set. > > However I can't seem to pinpoint a commit where this behavior was > introduced. It seems like it's been like that since forever. > > At a first glance it would make more sense to just handle the > BRIDGE_VLAN_INFO_PVID flag in __vlan_add_flags. However, as Nikolay > explains: > > There are a few reasons why we don't do it, most importantly because > we need to have only one visible pvid at any single time, even if it's > stale - it must be just one. Right now that rule will not be violated > by this change, but people will try using this flag and could see two > pvids simultaneously. You can see that the pvid code is even using > memory barriers to propagate the new value faster and everywhere the > pvid is read only once. That is the reason the flag is set > dynamically when dumping entries, too. A second (weaker) argument > against would be given the above we don't want another way to do the > same thing, specifically if it can provide us with two pvids (e.g. if > walking the vlan list) or if it can provide us with a pvid different > from the one set in the vg. [Obviously, I'm talking about RCU > pvid/vlan use cases similar to the dumps. The locked cases are fine. > I would like to avoid explaining why this shouldn't be relied upon > without locking] > > So instead of introducing the above change and making sure of the pvid > uniqueness under RCU, simply dynamically populate the pvid flag in > br_vlan_get_info(). > > Signed-off-by: Vladimir Oltean <olte...@gmail.com> > --- > net/bridge/br_vlan.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > index f5b2aeebbfe9..bb98984cd27d 100644 > --- a/net/bridge/br_vlan.c > +++ b/net/bridge/br_vlan.c > @@ -1281,6 +1281,8 @@ int br_vlan_get_info(const struct net_device *dev, u16 > vid, > > p_vinfo->vid = vid; > p_vinfo->flags = v->flags; > + if (vid == br_get_pvid(vg)) > + p_vinfo->flags |= BRIDGE_VLAN_INFO_PVID; > return 0; > } > EXPORT_SYMBOL_GPL(br_vlan_get_info); >
Looks good, thanks! Acked-by: Nikolay Aleksandrov <niko...@cumulusnetworks.com>
