| Message ID | 1401165586-13836-1-git-send-email-roopa@cumulusnetworks.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 26 May 2014 21:39:46 -0700 roopa@cumulusnetworks.com wrote: > From: Roopa Prabhu <roopa@cumulusnetworks.com> > > This patch adds NDA_MASTER attribute to neighbour attributes enum for > bridge/master ifindex. And adds NDA_MASTER to bridge fdb notify msgs. > > Today bridge fdb notifications dont contain bridge information. > Userspace can derive it from the port information in the fdb > notification. However this is tricky in some scenarious. > > Example, bridge port delete notification comes before bridge fdb > delete notifications. And we have seen problems in userspace > when using libnl where, the bridge fdb delete notification handling code > does not understand which bridge this fdb entry is part of because > the bridge and port association has already been deleted. > And these notifications (port membership and fdb) are generated on > separate rtnl groups. > > Fixing the order of notifications could possibly solve the problem > for some cases (I can submit a separate patch for that). > > This patch chooses to add NDA_MASTER to bridge fdb notify msgs > because it not only solves the problem described above, but also helps > userspace avoid another lookup into link msgs to derive the master index. > > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> > --- > include/uapi/linux/neighbour.h | 1 + > net/bridge/br_fdb.c | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h > index d3ef583..4a1d7e9 100644 > --- a/include/uapi/linux/neighbour.h > +++ b/include/uapi/linux/neighbour.h > @@ -24,6 +24,7 @@ enum { > NDA_PORT, > NDA_VNI, > NDA_IFINDEX, > + NDA_MASTER, > __NDA_MAX > }; > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index 9203d5a..019bb93 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -565,6 +565,8 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br, > > if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->addr)) > goto nla_put_failure; > + if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex)) > + goto nla_put_failure; > ci.ndm_used = jiffies_to_clock_t(now - fdb->used); > ci.ndm_confirmed = 0; > ci.ndm_updated = jiffies_to_clock_t(now - fdb->updated); > @@ -586,6 +588,7 @@ static inline size_t fdb_nlmsg_size(void) > { > return NLMSG_ALIGN(sizeof(struct ndmsg)) > + nla_total_size(ETH_ALEN) /* NDA_LLADDR */ > + + nla_total_size(sizeof(u32)) /* NDA_MASTER */ > + nla_total_size(sizeof(u16)) /* NDA_VLAN */ > + nla_total_size(sizeof(struct nda_cacheinfo)); > } I like the idea of this, but the new attribute needs to be part of the set as well as notify and display infrastructure. -- 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
On 5/27/14, 9:37 AM, Stephen Hemminger wrote: > On Mon, 26 May 2014 21:39:46 -0700 > roopa@cumulusnetworks.com wrote: > >> From: Roopa Prabhu <roopa@cumulusnetworks.com> >> >> This patch adds NDA_MASTER attribute to neighbour attributes enum for >> bridge/master ifindex. And adds NDA_MASTER to bridge fdb notify msgs. >> >> Today bridge fdb notifications dont contain bridge information. >> Userspace can derive it from the port information in the fdb >> notification. However this is tricky in some scenarious. >> >> Example, bridge port delete notification comes before bridge fdb >> delete notifications. And we have seen problems in userspace >> when using libnl where, the bridge fdb delete notification handling code >> does not understand which bridge this fdb entry is part of because >> the bridge and port association has already been deleted. >> And these notifications (port membership and fdb) are generated on >> separate rtnl groups. >> >> Fixing the order of notifications could possibly solve the problem >> for some cases (I can submit a separate patch for that). >> >> This patch chooses to add NDA_MASTER to bridge fdb notify msgs >> because it not only solves the problem described above, but also helps >> userspace avoid another lookup into link msgs to derive the master index. >> >> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >> --- >> include/uapi/linux/neighbour.h | 1 + >> net/bridge/br_fdb.c | 3 +++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h >> index d3ef583..4a1d7e9 100644 >> --- a/include/uapi/linux/neighbour.h >> +++ b/include/uapi/linux/neighbour.h >> @@ -24,6 +24,7 @@ enum { >> NDA_PORT, >> NDA_VNI, >> NDA_IFINDEX, >> + NDA_MASTER, >> __NDA_MAX >> }; >> >> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c >> index 9203d5a..019bb93 100644 >> --- a/net/bridge/br_fdb.c >> +++ b/net/bridge/br_fdb.c >> @@ -565,6 +565,8 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br, >> >> if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->addr)) >> goto nla_put_failure; >> + if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex)) >> + goto nla_put_failure; >> ci.ndm_used = jiffies_to_clock_t(now - fdb->used); >> ci.ndm_confirmed = 0; >> ci.ndm_updated = jiffies_to_clock_t(now - fdb->updated); >> @@ -586,6 +588,7 @@ static inline size_t fdb_nlmsg_size(void) >> { >> return NLMSG_ALIGN(sizeof(struct ndmsg)) >> + nla_total_size(ETH_ALEN) /* NDA_LLADDR */ >> + + nla_total_size(sizeof(u32)) /* NDA_MASTER */ >> + nla_total_size(sizeof(u16)) /* NDA_VLAN */ >> + nla_total_size(sizeof(struct nda_cacheinfo)); >> } > I like the idea of this, but the new attribute needs to be part of the set > as well as notify and display infrastructure. > ok thanks, i will resubmit with set and other changes. -- 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
On 05/27/14 13:05, Roopa Prabhu wrote: >> I like the idea of this, but the new attribute needs to be part of the >> set >> as well as notify and display infrastructure. >> > ok thanks, i will resubmit with set and other changes. I think it is useful for symettry purposes to have both directions have NDA_MASTER; but other than that, I dont see any purpose NDA_MASTER serves. A bridge port is specified on the ndm msg to the kernel. A bridge port can only belong to one master. The kernel can deduce that already. Infact i think specifying the NDA_MASTER may cause problems when the specified NDA_MASTER is not the bridge to which the bridge port belongs to.... cheers, jamal -- 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
Just to be clear - I meant i dont see its usefulness in a set (definitely useful in notify and get/dump). cheers, jamal On 05/27/14 17:51, Jamal Hadi Salim wrote: > I think it is useful for symettry purposes to have both directions > have NDA_MASTER; but other than that, I dont see any purpose NDA_MASTER > serves. A bridge port is specified on the ndm msg to the kernel. > A bridge port can only belong to one master. > The kernel can deduce that already. > Infact i think specifying the NDA_MASTER may cause problems when > the specified NDA_MASTER is not the bridge to which the bridge port > belongs to.... > > cheers, > jamal > -- 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
Jamal, i hadn't looked at NDA_MASTER for set yet. I was going to. We have some versions of patches for notify and dump which i was mainly focusing on. Agree that it is not needed for sets and creates further confusion and possibly creates the same problems in userspace which i am trying to solve. So, ack on that. I had a question regarding dump, We can filter in kernel (as your patch does on the other thread) or in userspace based on master index with new filter arguments to iproute2 to determine the bridge and port for filtering. This follows the existing filtering support in all other cmds in iproute2. Which is great. But, Is there any interest in adding master to the default iproute2 bridge output ?. like the below ? # bridge fdb show 44:38:39:00:27:ba dev bond2.2003 master br-2003 permanent 44:38:39:00:27:bb dev bond4.2003 master br-2003 permanent 44:38:39:00:27:bc dev bond2.2004 master br-2004 permanent master can be put at the end of the output line for each fdb entry or make it optional with -d[etails]. (Don't intend to change output and break existing apps and i also understand that filtering by bridge/master name is a way to solve the problem. But i had a request from our internal team to post the question. So, just asking to see if there is interest to modify the default fdb show to include the master during display. It would make the default global fdb show cmd more complete). Thanks, Roopa On 5/27/14, 2:57 PM, Jamal Hadi Salim wrote: > Just to be clear - I meant i dont see its usefulness in a set > (definitely useful in notify and get/dump). > > cheers, > jamal > > On 05/27/14 17:51, Jamal Hadi Salim wrote: > >> I think it is useful for symettry purposes to have both directions >> have NDA_MASTER; but other than that, I dont see any purpose NDA_MASTER >> serves. A bridge port is specified on the ndm msg to the kernel. >> A bridge port can only belong to one master. >> The kernel can deduce that already. >> Infact i think specifying the NDA_MASTER may cause problems when >> the specified NDA_MASTER is not the bridge to which the bridge port >> belongs to.... >> >> cheers, >> jamal >> > -- 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
On 05/27/14 20:05, Roopa Prabhu wrote: > Jamal, > > i hadn't looked at NDA_MASTER for set yet. > I was going to. We have some versions of patches for notify and dump > which i was mainly focusing on. > Agree that it is not needed for sets and creates further confusion and > possibly creates the same problems in userspace which i am trying to > solve. So, ack on that. > Ok. > I had a question regarding dump, > We can filter in kernel (as your patch does on the other thread) or in > userspace based on master index with new filter arguments to iproute2 to > determine the bridge and port for filtering. This follows the existing > filtering support in all other cmds in iproute2. Which is great. > I was thinking of doing what you did after - if i was able to show you my handwritten notes, youd see it scribbled ;-> So i am glad you did. > But, Is there any interest in adding master to the default iproute2 > bridge output ?. like the below ? > # bridge fdb show > 44:38:39:00:27:ba dev bond2.2003 master br-2003 permanent > 44:38:39:00:27:bb dev bond4.2003 master br-2003 permanent > 44:38:39:00:27:bc dev bond2.2004 master br-2004 permanent > > master can be put at the end of the output line for each fdb entry or > make it optional with -d[etails]. > > > (Don't intend to change output and break existing apps and i also > understand that filtering by bridge/master name is a way to solve the > problem. But i had a request from our internal team to post the > question. So, just asking to see if there is interest to modify the > default fdb show to include the master during display. It would make the > default global fdb show cmd more complete). I agree that is more complete to just display the bridge as well. Not sure even -d should be necessary. The bridge command is so new i am not sure people will scream - but i leave that call to Stephen. cheers, jamal PS:- I will redo my patch on top of yours and use ndm ifindex for bridge port. -- 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
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h index d3ef583..4a1d7e9 100644 --- a/include/uapi/linux/neighbour.h +++ b/include/uapi/linux/neighbour.h @@ -24,6 +24,7 @@ enum { NDA_PORT, NDA_VNI, NDA_IFINDEX, + NDA_MASTER, __NDA_MAX }; diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index 9203d5a..019bb93 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -565,6 +565,8 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br, if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->addr)) goto nla_put_failure; + if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex)) + goto nla_put_failure; ci.ndm_used = jiffies_to_clock_t(now - fdb->used); ci.ndm_confirmed = 0; ci.ndm_updated = jiffies_to_clock_t(now - fdb->updated); @@ -586,6 +588,7 @@ static inline size_t fdb_nlmsg_size(void) { return NLMSG_ALIGN(sizeof(struct ndmsg)) + nla_total_size(ETH_ALEN) /* NDA_LLADDR */ + + nla_total_size(sizeof(u32)) /* NDA_MASTER */ + nla_total_size(sizeof(u16)) /* NDA_VLAN */ + nla_total_size(sizeof(struct nda_cacheinfo)); }