Re: [tipc-discussion] [net-next 02/16] tipc: move creation of publication item one level up in call chain

[Xin L. <luc...@gm...>]

On Wed, Dec 9, 2020 at 2:51 AM <jm...@re...> wrote:
>
> From: Jon Maloy <jm...@re...>
>
> We instantiante struct publication in tipc_nametbl_insert_publ()
> instead of as currently in tipc_service_insert_publ(). This has the
> advantage that we can pass a pointer to the publication struct to
> the next call levels, instead of the numerous individual parameters
> we pass on now. It also gives us a location to keep the contents of
> the additional fields we will introduce in a later commit.
>
> Signed-off-by: Jon Maloy <jm...@re...>
> ---
>  net/tipc/name_table.c | 63 ++++++++++++++++++++++---------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
>
> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> index c37a4a9c87ca..68e269b49780 100644
> --- a/net/tipc/name_table.c
> +++ b/net/tipc/name_table.c
> @@ -327,49 +327,44 @@ static struct service_range *tipc_service_create_range(struct tipc_service *sc,
>         return sr;
>  }
>
> -static struct publication *tipc_service_insert_publ(struct net *net,
> -                                                   struct tipc_service *sc,
> -                                                   u32 type, u32 lower,
> -                                                   u32 upper, u32 scope,
> -                                                   u32 node, u32 port,
> -                                                   u32 key)
> +static bool tipc_service_insert_publ(struct net *net,
> +                                    struct tipc_service *sc,
> +                                    struct publication *p)
>  {
>         struct tipc_subscription *sub, *tmp;
>         struct service_range *sr;
> -       struct publication *p;
> +       struct publication *_p;
> +       u32 node = p->sk.node;
>         bool first = false;
>
> -       sr = tipc_service_create_range(sc, lower, upper);
> +       sr = tipc_service_create_range(sc, p->sr.lower, p->sr.upper);
>         if (!sr)
>                 goto  err;
>
>         first = list_empty(&sr->all_publ);
>
>         /* Return if the publication already exists */
> -       list_for_each_entry(p, &sr->all_publ, all_publ) {
> -               if (p->key == key && (!p->sk.node || p->sk.node == node))
> -                       return NULL;
> +       list_for_each_entry(_p, &sr->all_publ, all_publ) {
> +               if (_p->key == p->key && (!_p->sk.node || _p->sk.node == node))
> +                       return false;
>         }
>
> -       /* Create and insert publication */
> -       p = tipc_publ_create(type, lower, upper, scope, node, port, key);
> -       if (!p)
> -               goto err;
> -       /* Suppose there shouldn't be a huge gap btw publs i.e. >INT_MAX */
> -       p->id = sc->publ_cnt++;
> -       if (in_own_node(net, node))
> +       if (in_own_node(net, p->sk.node))
>                 list_add(&p->local_publ, &sr->local_publ);
>         list_add(&p->all_publ, &sr->all_publ);
> +       p->id = sc->publ_cnt++;
>
>         /* Any subscriptions waiting for notification?  */
>         list_for_each_entry_safe(sub, tmp, &sc->subscriptions, service_list) {
> -               tipc_sub_report_overlap(sub, p->sr.lower, p->sr.upper, TIPC_PUBLISHED,
> -                                       p->sk.ref, p->sk.node, p->scope, first);
> +               tipc_sub_report_overlap(sub, p->sr.lower, p->sr.upper,
> +                                       TIPC_PUBLISHED, p->sk.ref, p->sk.node,
> +                                       p->scope, first);
>         }
> -       return p;
> +       return true;
>  err:
> -       pr_warn("Failed to bind to %u,%u,%u, no memory\n", type, lower, upper);
> -       return NULL;
> +       pr_warn("Failed to bind to %u,%u,%u, no memory\n",
> +               p->sr.type, p->sr.lower, p->sr.upper);
> +       return false;
>  }
>
>  /**
> @@ -481,6 +476,11 @@ struct publication *tipc_nametbl_insert_publ(struct net *net, u32 type,
>         struct name_table *nt = tipc_name_table(net);
>         struct tipc_service *sc;
>         struct publication *p;
> +       bool res = false;
> +
> +       p = tipc_publ_create(type, lower, upper, scope, node, port, key);
> +       if (!p)
> +               return NULL;
>
>         if (scope > TIPC_NODE_SCOPE || lower > upper) {
>                 pr_debug("Failed to bind illegal {%u,%u,%u} with scope %u\n",
> @@ -490,14 +490,15 @@ struct publication *tipc_nametbl_insert_publ(struct net *net, u32 type,
>         sc = tipc_service_find(net, type);
>         if (!sc)
>                 sc = tipc_service_create(type, &nt->services[hash(type)]);
> -       if (!sc)
> -               return NULL;
> -
> -       spin_lock_bh(&sc->lock);
> -       p = tipc_service_insert_publ(net, sc, type, lower, upper,
> -                                    scope, node, port, key);
> -       spin_unlock_bh(&sc->lock);
> -       return p;
> +       if (sc) {
> +               spin_lock_bh(&sc->lock);
> +               res = tipc_service_insert_publ(net, sc, p);
> +               spin_unlock_bh(&sc->lock);
If move spin_(un)lock_bh into tipc_service_insert_publ(), we can do:

        if (sc && tipc_service_insert_publ(net, sc, p))
                return p;

> +       }
> +       if (res)
> +               return p;
and remove 'bool res = false;' and this 'if'.

> +       kfree(p);
> +       return NULL;
>  }
>
>  struct publication *tipc_nametbl_remove_publ(struct net *net, u32 type,
> --
> 2.28.0
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion