Re: [PATCH 19.11 3/4] net/hns3: fix multi-process action register and unregister

[Christian Ehrhardt <christian.ehrhardt@canonical.com>]

On Sat, Dec 25, 2021 at 11:58 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> [ upstream commit 841f8693536f9410fd51d385e1090d35cfe59914 ]
>

Thank you, queued for 19.11.12 (later this year)

> The multi-process has the following problems:
> 1) After a port in primary process is closed, the mp action of the
>    process is unregistered. Which will cause that other device in the
>    primary process cannot respond to requests from secondary processes.
> 2) Because variable "hns3_inited" is set to true without returning an
>    initial value, the mp action cannot be registered again after it is
>    unregistered.
> 3) The mp action of primary and secondary process need to be registered
>    only once regardless of port numbers in the process. That's what
>    variable "hns3_inited" does. But the variable is difficult to
>    understand.
>
> This patch adds a hns3_process_local_data structure to resolve above
> problems.
>
> Fixes: 9570b1fdbdad ("net/hns3: check multi-process action register result")
> Fixes: 23d4b61fee5d ("net/hns3: support multiple process")
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/net/hns3/hns3_ethdev.c    |  2 ++
>  drivers/net/hns3/hns3_ethdev_vf.c |  2 ++
>  drivers/net/hns3/hns3_mp.c        | 37 ++++++++++++++++++-------------
>  drivers/net/hns3/hns3_mp.h        |  7 ++++++
>  4 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
> index d245c5db8b..122a2bc66c 100644
> --- a/drivers/net/hns3/hns3_ethdev.c
> +++ b/drivers/net/hns3/hns3_ethdev.c
> @@ -5439,6 +5439,7 @@ hns3_dev_init(struct rte_eth_dev *eth_dev)
>                         goto err_mp_init_secondary;
>                 }
>                 __atomic_fetch_add(&hw->secondary_cnt, 1, __ATOMIC_RELAXED);
> +               process_data.eth_dev_cnt++;
>                 return 0;
>         }
>
> @@ -5449,6 +5450,7 @@ hns3_dev_init(struct rte_eth_dev *eth_dev)
>                              ret);
>                 goto err_mp_init_primary;
>         }
> +       process_data.eth_dev_cnt++;
>
>         hw->adapter_state = HNS3_NIC_UNINITIALIZED;
>
> diff --git a/drivers/net/hns3/hns3_ethdev_vf.c b/drivers/net/hns3/hns3_ethdev_vf.c
> index 972a6f00e4..562f6f7662 100644
> --- a/drivers/net/hns3/hns3_ethdev_vf.c
> +++ b/drivers/net/hns3/hns3_ethdev_vf.c
> @@ -2294,6 +2294,7 @@ hns3vf_dev_init(struct rte_eth_dev *eth_dev)
>                         goto err_mp_init_secondary;
>                 }
>                 __atomic_fetch_add(&hw->secondary_cnt, 1, __ATOMIC_RELAXED);
> +               process_data.eth_dev_cnt++;
>                 return 0;
>         }
>
> @@ -2304,6 +2305,7 @@ hns3vf_dev_init(struct rte_eth_dev *eth_dev)
>                              ret);
>                 goto err_mp_init_primary;
>         }
> +       process_data.eth_dev_cnt++;
>
>         hw->adapter_state = HNS3_NIC_UNINITIALIZED;
>         hns->is_vf = true;
> diff --git a/drivers/net/hns3/hns3_mp.c b/drivers/net/hns3/hns3_mp.c
> index 9b5ff475a9..c7e49a798a 100644
> --- a/drivers/net/hns3/hns3_mp.c
> +++ b/drivers/net/hns3/hns3_mp.c
> @@ -14,7 +14,8 @@
>  #include "hns3_rxtx.h"
>  #include "hns3_mp.h"
>
> -static bool hns3_inited;
> +/* local data for primary or secondary process. */
> +struct hns3_process_local_data process_data;
>
>  /*
>   * Initialize IPC message.
> @@ -199,14 +200,15 @@ int hns3_mp_init_primary(void)
>  {
>         int ret;
>
> -       if (!hns3_inited) {
> -               /* primary is allowed to not support IPC */
> -               ret = rte_mp_action_register(HNS3_MP_NAME, mp_primary_handle);
> -               if (ret && rte_errno != ENOTSUP)
> -                       return ret;
> +       if (process_data.init_done)
> +               return 0;
>
> -               hns3_inited = true;
> -       }
> +       /* primary is allowed to not support IPC */
> +       ret = rte_mp_action_register(HNS3_MP_NAME, mp_primary_handle);
> +       if (ret && rte_errno != ENOTSUP)
> +               return ret;
> +
> +       process_data.init_done = true;
>
>         return 0;
>  }
> @@ -216,8 +218,12 @@ int hns3_mp_init_primary(void)
>   */
>  void hns3_mp_uninit_primary(void)
>  {
> -       if (hns3_inited)
> +       process_data.eth_dev_cnt--;
> +
> +       if (process_data.eth_dev_cnt == 0) {
>                 rte_mp_action_unregister(HNS3_MP_NAME);
> +               process_data.init_done = false;
> +       }
>  }
>
>  /*
> @@ -227,13 +233,14 @@ int hns3_mp_init_secondary(void)
>  {
>         int ret;
>
> -       if (!hns3_inited) {
> -               ret = rte_mp_action_register(HNS3_MP_NAME, mp_secondary_handle);
> -               if (ret)
> -                       return ret;
> +       if (process_data.init_done)
> +               return 0;
>
> -               hns3_inited = true;
> -       }
> +       ret = rte_mp_action_register(HNS3_MP_NAME, mp_secondary_handle);
> +       if (ret)
> +               return ret;
> +
> +       process_data.init_done = true;
>
>         return 0;
>  }
> diff --git a/drivers/net/hns3/hns3_mp.h b/drivers/net/hns3/hns3_mp.h
> index 60ef2315db..8ef432e763 100644
> --- a/drivers/net/hns3/hns3_mp.h
> +++ b/drivers/net/hns3/hns3_mp.h
> @@ -5,6 +5,13 @@
>  #ifndef _HNS3_MP_H_
>  #define _HNS3_MP_H_
>
> +/* Local data for primary or secondary process. */
> +struct hns3_process_local_data {
> +       bool init_done; /* Process action register completed flag. */
> +       int eth_dev_cnt; /* Ethdev count under the current process. */
> +};
> +extern struct hns3_process_local_data process_data;
> +
>  void hns3_mp_req_start_rxtx(struct rte_eth_dev *dev);
>  void hns3_mp_req_stop_rxtx(struct rte_eth_dev *dev);
>  int hns3_mp_init_primary(void);
> --
> 2.33.0
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd