| Message ID | 20200908091037.2709823-4-idosch@idosch.org |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | nexthop: Add support for nexthop objects offload | expand |
On 9/8/20 3:10 AM, Ido Schimmel wrote: > From: Ido Schimmel <idosch@nvidia.com> > > Currently, the delete notification is emitted from the error path of > nexthop_add() and replace_nexthop(), which can be confusing to listeners > as they are not familiar with the nexthop. > > Instead, only emit the notification when the nexthop is actually > deleted. The following sub-cases are covered: > > 1. User space deletes the nexthop > 2. The nexthop is deleted by the kernel due to a netdev event (e.g., > nexthop device going down) > 3. A group is deleted because its last nexthop is being deleted > 4. The network namespace of the nexthop device is deleted > > Signed-off-by: Ido Schimmel <idosch@nvidia.com> > --- > net/ipv4/nexthop.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: David Ahern <dsahern@gmail.com>
Tue, Sep 08, 2020 at 11:10:18AM CEST, idosch@idosch.org wrote: >From: Ido Schimmel <idosch@nvidia.com> > >Currently, the delete notification is emitted from the error path of >nexthop_add() and replace_nexthop(), which can be confusing to listeners >as they are not familiar with the nexthop. > >Instead, only emit the notification when the nexthop is actually >deleted. The following sub-cases are covered: Well, in theory, this might break some very odd app that is adding a route and checking the errors using this notification. My opinion is to allow this breakage to happen, but I'm usually too benevolent :) > >1. User space deletes the nexthop >2. The nexthop is deleted by the kernel due to a netdev event (e.g., > nexthop device going down) >3. A group is deleted because its last nexthop is being deleted >4. The network namespace of the nexthop device is deleted > >Signed-off-by: Ido Schimmel <idosch@nvidia.com> >--- > net/ipv4/nexthop.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c >index 13d9219a9aa1..8c0f17c6863c 100644 >--- a/net/ipv4/nexthop.c >+++ b/net/ipv4/nexthop.c >@@ -870,8 +870,6 @@ static void __remove_nexthop_fib(struct net *net, struct nexthop *nh) > bool do_flush = false; > struct fib_info *fi; > >- call_nexthop_notifiers(net, NEXTHOP_EVENT_DEL, nh); >- > list_for_each_entry(fi, &nh->fi_list, nh_list) { > fi->fib_flags |= RTNH_F_DEAD; > do_flush = true; >@@ -909,6 +907,8 @@ static void __remove_nexthop(struct net *net, struct nexthop *nh, > static void remove_nexthop(struct net *net, struct nexthop *nh, > struct nl_info *nlinfo) > { >+ call_nexthop_notifiers(net, NEXTHOP_EVENT_DEL, nh); >+ > /* remove from the tree */ > rb_erase(&nh->rb_node, &net->nexthop.rb_root); > >-- >2.26.2 >
On 9/8/20 8:39 AM, Jiri Pirko wrote: > Tue, Sep 08, 2020 at 11:10:18AM CEST, idosch@idosch.org wrote: >> From: Ido Schimmel <idosch@nvidia.com> >> >> Currently, the delete notification is emitted from the error path of >> nexthop_add() and replace_nexthop(), which can be confusing to listeners >> as they are not familiar with the nexthop. >> >> Instead, only emit the notification when the nexthop is actually >> deleted. The following sub-cases are covered: > > Well, in theory, this might break some very odd app that is adding a > route and checking the errors using this notification. My opinion is to > allow this breakage to happen, but I'm usually too benevolent :) > That would be a very twisted app. No other notifications go out on failure to add / replace and nexthops should be consistent.
On Tue, Sep 08, 2020 at 04:39:59PM +0200, Jiri Pirko wrote: > Tue, Sep 08, 2020 at 11:10:18AM CEST, idosch@idosch.org wrote: > >From: Ido Schimmel <idosch@nvidia.com> > > > >Currently, the delete notification is emitted from the error path of > >nexthop_add() and replace_nexthop(), which can be confusing to listeners > >as they are not familiar with the nexthop. > > > >Instead, only emit the notification when the nexthop is actually > >deleted. The following sub-cases are covered: > > Well, in theory, this might break some very odd app that is adding a > route and checking the errors using this notification. My opinion is to > allow this breakage to happen, but I'm usually too benevolent :) There is no uAPI breakage since the patch is only changing the in-kernel notification. After this patch the in-kernel and RTM_DELNEXTHOP netlink notifications are both emitted from the same function (i.e., remove_nexthop()). I'll reword the commit message to make it clear. > > > > > >1. User space deletes the nexthop > >2. The nexthop is deleted by the kernel due to a netdev event (e.g., > > nexthop device going down) > >3. A group is deleted because its last nexthop is being deleted > >4. The network namespace of the nexthop device is deleted > > > >Signed-off-by: Ido Schimmel <idosch@nvidia.com> > >--- > > net/ipv4/nexthop.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c > >index 13d9219a9aa1..8c0f17c6863c 100644 > >--- a/net/ipv4/nexthop.c > >+++ b/net/ipv4/nexthop.c > >@@ -870,8 +870,6 @@ static void __remove_nexthop_fib(struct net *net, struct nexthop *nh) > > bool do_flush = false; > > struct fib_info *fi; > > > >- call_nexthop_notifiers(net, NEXTHOP_EVENT_DEL, nh); > >- > > list_for_each_entry(fi, &nh->fi_list, nh_list) { > > fi->fib_flags |= RTNH_F_DEAD; > > do_flush = true; > >@@ -909,6 +907,8 @@ static void __remove_nexthop(struct net *net, struct nexthop *nh, > > static void remove_nexthop(struct net *net, struct nexthop *nh, > > struct nl_info *nlinfo) > > { > >+ call_nexthop_notifiers(net, NEXTHOP_EVENT_DEL, nh); > >+ > > /* remove from the tree */ > > rb_erase(&nh->rb_node, &net->nexthop.rb_root); > > > >-- > >2.26.2 > >
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c index 13d9219a9aa1..8c0f17c6863c 100644 --- a/net/ipv4/nexthop.c +++ b/net/ipv4/nexthop.c @@ -870,8 +870,6 @@ static void __remove_nexthop_fib(struct net *net, struct nexthop *nh) bool do_flush = false; struct fib_info *fi; - call_nexthop_notifiers(net, NEXTHOP_EVENT_DEL, nh); - list_for_each_entry(fi, &nh->fi_list, nh_list) { fi->fib_flags |= RTNH_F_DEAD; do_flush = true; @@ -909,6 +907,8 @@ static void __remove_nexthop(struct net *net, struct nexthop *nh, static void remove_nexthop(struct net *net, struct nexthop *nh, struct nl_info *nlinfo) { + call_nexthop_notifiers(net, NEXTHOP_EVENT_DEL, nh); + /* remove from the tree */ rb_erase(&nh->rb_node, &net->nexthop.rb_root);