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:
> >
>
