Re: [RFC PATCH net-next 02/15] ipv4: Notify route after insertion to the routing table

[Ido Schimmel <idosch@idosch.org>]

On Wed, Oct 02, 2019 at 07:34:35PM -0600, David Ahern wrote:
> On 10/2/19 2:40 AM, Ido Schimmel wrote:
> > @@ -1269,14 +1269,19 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
> >  	new_fa->tb_id = tb->tb_id;
> >  	new_fa->fa_default = -1;
> >  
> > -	err = call_fib_entry_notifiers(net, event, key, plen, new_fa, extack);
> > +	/* Insert new entry to the list. */
> > +	err = fib_insert_alias(t, tp, l, new_fa, fa, key);
> >  	if (err)
> >  		goto out_free_new_fa;
> >  
> > -	/* Insert new entry to the list. */
> > -	err = fib_insert_alias(t, tp, l, new_fa, fa, key);
> > +	/* The alias was already inserted, so the node must exist. */
> > +	l = fib_find_node(t, &tp, key);
> > +	if (WARN_ON_ONCE(!l))
> > +		goto out_free_new_fa;
> 
> Maybe I am missing something but, the 'l' is only needed for the error
> path, so optimize for the success case and only lookup the node if the
> notifier fails.

Hi David,

You're correct that in this patch it is only needed by the error path, but
later on in patch 4 ("ipv4: Notify newly added route if should be
offloaded") we actually need it here as well. The FIB node can be NULL
and fib_insert_alias() will create one if that's the case.

> 
> > +
> > +	err = call_fib_entry_notifiers(net, event, key, plen, new_fa, extack);
> >  	if (err)
> > -		goto out_fib_notif;
> > +		goto out_remove_new_fa;
> >  
> >  	if (!plen)
> >  		tb->tb_num_default++;
> > @@ -1287,14 +1292,8 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
> >  succeeded:
> >  	return 0;
> >  
> > -out_fib_notif:
> > -	/* notifier was sent that entry would be added to trie, but
> > -	 * the add failed and need to recover. Only failure for
> > -	 * fib_insert_alias is ENOMEM.
> > -	 */
> > -	NL_SET_ERR_MSG(extack, "Failed to insert route into trie");
> > -	call_fib_entry_notifiers(net, FIB_EVENT_ENTRY_DEL, key,
> > -				 plen, new_fa, NULL);
> > +out_remove_new_fa:
> > +	fib_remove_alias(t, tp, l, new_fa);
> >  out_free_new_fa:
> >  	kmem_cache_free(fn_alias_kmem, new_fa);
> >  out:
> > 
>