From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [patch net-next 2/7] ipv4: ipmr: Add the parent ID field to VIF struct Date: Fri, 29 Sep 2017 12:45:17 +0300 Message-ID: <11d1be00-0bd4-3bcc-d63b-9419d5399602@cumulusnetworks.com> References: <20170928173415.15551-1-jiri@resnulli.us> <20170928173415.15551-3-jiri@resnulli.us> <7618b2e8-e7f9-d2c6-b13c-53aef0f50de0@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, yotamg@mellanox.com, idosch@mellanox.com, mlxsw@mellanox.com, andrew@lunn.ch, dsa@cumulusnetworks.com, edumazet@google.com, willemb@google.com, johannes.berg@intel.com, dcaratti@redhat.com, pabeni@redhat.com, daniel@iogearbox.net, f.fainelli@gmail.com, fw@strlen.de, gfree.wind@vip.163.com To: Jiri Pirko , netdev@vger.kernel.org Return-path: Received: from mail-wm0-f41.google.com ([74.125.82.41]:55873 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750926AbdI2JpV (ORCPT ); Fri, 29 Sep 2017 05:45:21 -0400 Received: by mail-wm0-f41.google.com with SMTP id u138so2143507wmu.4 for ; Fri, 29 Sep 2017 02:45:20 -0700 (PDT) In-Reply-To: <7618b2e8-e7f9-d2c6-b13c-53aef0f50de0@cumulusnetworks.com> Sender: netdev-owner@vger.kernel.org List-ID: On 29/09/17 12:29, Nikolay Aleksandrov wrote: > On 28/09/17 20:34, Jiri Pirko wrote: >> From: Yotam Gigi >> >> In order to allow the ipmr module to do partial multicast forwarding >> according to the device parent ID, add the device parent ID field to the >> VIF struct. This way, the forwarding path can use the parent ID field >> without invoking switchdev calls, which requires the RTNL lock. >> >> When a new VIF is added, set the device parent ID field in it by invoking >> the switchdev_port_attr_get call. >> >> Signed-off-by: Yotam Gigi >> Reviewed-by: Ido Schimmel >> Signed-off-by: Jiri Pirko >> --- >> include/linux/mroute.h | 2 ++ >> net/ipv4/ipmr.c | 9 +++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/include/linux/mroute.h b/include/linux/mroute.h >> index b072a84..a46577f 100644 >> --- a/include/linux/mroute.h >> +++ b/include/linux/mroute.h >> @@ -57,6 +57,8 @@ static inline bool ipmr_rule_default(const struct fib_rule *rule) >> >> struct vif_device { >> struct net_device *dev; /* Device we are using */ >> + struct netdev_phys_item_id dev_parent_id; /* Device parent ID */ >> + bool dev_parent_id_valid; >> unsigned long bytes_in,bytes_out; >> unsigned long pkt_in,pkt_out; /* Statistics */ >> unsigned long rate_limit; /* Traffic shaping (NI) */ >> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c >> index 292a8e8..4566c54 100644 >> --- a/net/ipv4/ipmr.c >> +++ b/net/ipv4/ipmr.c >> @@ -67,6 +67,7 @@ >> #include >> #include >> #include >> +#include >> >> struct ipmr_rule { >> struct fib_rule common; >> @@ -868,6 +869,9 @@ static int vif_add(struct net *net, struct mr_table *mrt, >> struct vifctl *vifc, int mrtsock) >> { >> int vifi = vifc->vifc_vifi; >> + struct switchdev_attr attr = { >> + .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID, >> + }; >> struct vif_device *v = &mrt->vif_table[vifi]; >> struct net_device *dev; >> struct in_device *in_dev; >> @@ -942,6 +946,11 @@ static int vif_add(struct net *net, struct mr_table *mrt, >> >> /* Fill in the VIF structures */ >> >> + attr.orig_dev = dev; >> + if (!switchdev_port_attr_get(dev, &attr)) { >> + v->dev_parent_id_valid = true; >> + memcpy(v->dev_parent_id.id, attr.u.ppid.id, attr.u.ppid.id_len); > > Hmm, shouldn't you set dev_parent_id.id_len too ? It would seem netdev_phys_item_id_same() > uses it in the comparison and without the len it would always look like they're the same > because memcmp will simply return 0 with count = 0. Also maybe we can use the non-zero id_len as a signal that it was set and drop the dev_parent_id_valid field altogether, it would seem there's no valid reason to have id_len == 0 and yet expect a valid parent_id. > >> + } >> v->rate_limit = vifc->vifc_rate_limit; >> v->local = vifc->vifc_lcl_addr.s_addr; >> v->remote = vifc->vifc_rmt_addr.s_addr; >> >