| Message ID | 20191002084103.12138-3-idosch@idosch.org |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Simplify IPv4 route offload API | expand |
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. > + > + 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: >
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: > > >
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c index b9df9c09b84e..3ba63ebcfeef 100644 --- a/net/ipv4/fib_trie.c +++ b/net/ipv4/fib_trie.c @@ -1063,9 +1063,6 @@ static int fib_insert_node(struct trie *t, struct key_vector *tp, return -ENOMEM; } -/* fib notifier for ADD is sent before calling fib_insert_alias with - * the expectation that the only possible failure ENOMEM - */ static int fib_insert_alias(struct trie *t, struct key_vector *tp, struct key_vector *l, struct fib_alias *new, struct fib_alias *fa, t_key key) @@ -1118,6 +1115,9 @@ static bool fib_valid_key_len(u32 key, u8 plen, struct netlink_ext_ack *extack) return true; } +static void fib_remove_alias(struct trie *t, struct key_vector *tp, + struct key_vector *l, struct fib_alias *old); + /* Caller must hold RTNL. */ int fib_table_insert(struct net *net, struct fib_table *tb, struct fib_config *cfg, struct netlink_ext_ack *extack) @@ -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; + + 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: