Re: [RFC PATCH net-next 11/22] nexthop: Emit a notification when a nexthop is added

[Ido Schimmel <idosch@idosch.org>]

On Tue, Sep 08, 2020 at 09:21:08AM -0600, David Ahern wrote:
> On 9/8/20 3:10 AM, Ido Schimmel wrote:
> > From: Ido Schimmel <idosch@nvidia.com>
> > 
> > Emit a notification in the nexthop notification chain when a new nexthop
> > is added (not replaced). The nexthop can either be a new group or a
> > single nexthop.
> 
> Add a comment about why EVENT_REPLACE is generated on an 'added (not
> replaced)' event.

Reworded:

"
nexthop: Emit a notification when a nexthop is added

Emit a notification in the nexthop notification chain when a new nexthop
is added (not replaced). The nexthop can either be a new group or a
single nexthop.

The notification is sent after the nexthop is inserted into the
red-black tree, as listeners might need to callback into the nexthop
code with the nexthop ID in order to mark the nexthop as offloaded.

A 'REPLACE' notification is emitted instead of 'ADD' as the distinction
between the two is not important for in-kernel listeners. In case the
listener is not familiar with the encoded nexthop ID, it can simply
treat it as a new one. This is also consistent with the route offload
API.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
"

> 
> > 
> > The notification is sent after the nexthop is inserted into the
> > red-black tree, as listeners might need to callback into the nexthop
> > code with the nexthop ID in order to mark the nexthop as offloaded.
> > 
> > Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> > ---
> >  include/net/nexthop.h | 3 ++-
> >  net/ipv4/nexthop.c    | 6 +++++-
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/nexthop.h b/include/net/nexthop.h
> > index 4147681e86d2..6431ff8cdb89 100644
> > --- a/include/net/nexthop.h
> > +++ b/include/net/nexthop.h
> > @@ -106,7 +106,8 @@ struct nexthop {
> >  
> >  enum nexthop_event_type {
> >  	NEXTHOP_EVENT_ADD,
> 
> looks like the ADD event is not used and can be removed.

Right. I will remove it in a separate patch

> 
> > -	NEXTHOP_EVENT_DEL
> > +	NEXTHOP_EVENT_DEL,
> > +	NEXTHOP_EVENT_REPLACE,
> >  };
> >  
> >  struct nh_notifier_single_info {
> > diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> > index 71605c612458..1fa249facd46 100644
> > --- a/net/ipv4/nexthop.c
> > +++ b/net/ipv4/nexthop.c
> > @@ -1277,7 +1277,11 @@ static int insert_nexthop(struct net *net, struct nexthop *new_nh,
> >  
> >  	rb_link_node_rcu(&new_nh->rb_node, parent, pp);
> >  	rb_insert_color(&new_nh->rb_node, root);
> > -	rc = 0;
> > +
> > +	rc = call_nexthop_notifiers(net, NEXTHOP_EVENT_REPLACE, new_nh, extack);
> > +	if (rc)
> > +		rb_erase(&new_nh->rb_node, &net->nexthop.rb_root);
> > +
> >  out:
> >  	if (!rc) {
> >  		nh_base_seq_inc(net);
> > 
>