Re: [dpdk-dev] [PATCH v2 1/9] ethdev: refactor representor infrastructure

[Somnath Kotur <somnath.kotur@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 17001 bytes --]

On Thu, Jan 7, 2021 at 12:08 PM Xueming(Steven) Li <xuemingl@nvidia.com> wrote:
>
>
>
> >-----Original Message-----
> >From: Somnath Kotur <somnath.kotur@broadcom.com>
> >Sent: Thursday, January 7, 2021 2:32 PM
> >To: Xueming(Steven) Li <xuemingl@nvidia.com>
> >Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> ><ferruh.yigit@intel.com>; Andrew Rybchenko
> ><andrew.rybchenko@oktetlabs.ru>; Olivier Matz <olivier.matz@6wind.com>;
> >Slava Ovsiienko <viacheslavo@nvidia.com>; dev <dev@dpdk.org>; Asaf Penso
> ><asafp@nvidia.com>
> >Subject: Re: [dpdk-dev] [PATCH v2 1/9] ethdev: refactor representor
> >infrastructure
> >
> >On Wed, Jan 6, 2021 at 9:48 PM Xueming Li <xuemingl@nvidia.com> wrote:
> >>
> >> To support extended representor syntax, this patch refactor represntor
Please fix this Typo in 'representor' as well ...Thanks
> >> infrastructure:
> >> 1. introduces representor type enum
> >> 2. devargs representor port range extraction from partial value
> >>
> >> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> >> ---
> >>  drivers/net/bnxt/bnxt_ethdev.c        | 12 ++++
> >>  drivers/net/enic/enic_ethdev.c        |  7 ++
> >>  drivers/net/i40e/i40e_ethdev.c        |  8 +++
> >>  drivers/net/ixgbe/ixgbe_ethdev.c      |  8 +++
> >>  drivers/net/mlx5/linux/mlx5_os.c      | 11 ++++
> >>  lib/librte_ethdev/ethdev_private.c    | 93 ++++++++++++---------------
> >>  lib/librte_ethdev/ethdev_private.h    |  3 -
> >>  lib/librte_ethdev/rte_class_eth.c     |  4 +-
> >>  lib/librte_ethdev/rte_ethdev.c        |  5 +-
> >>  lib/librte_ethdev/rte_ethdev_driver.h |  7 ++
> >>  10 files changed, 98 insertions(+), 60 deletions(-)
> >>
> >> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> >b/drivers/net/bnxt/bnxt_ethdev.c
> >> index 81c8f8d79d..844a6c3c66 100644
> >> --- a/drivers/net/bnxt/bnxt_ethdev.c
> >> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> >> @@ -5520,6 +5520,18 @@ static int bnxt_rep_port_probe(struct
> >rte_pci_device *pci_dev,
> >>         int i, ret = 0;
> >>         struct rte_kvargs *kvlist = NULL;
> >>
> >> +       if (eth_da->type == RTE_ETH_REPRESENTOR_NONE)
> >> +               return 0;
> >> +       if (eth_da->type != RTE_ETH_REPRESENTOR_VF) {
> >> +               PMD_DRV_LOG(ERR, "unsupported representor type %d\n",
> >> +                           eth_da->type);
> >> +               return -ENOTSUP;
> >> +       }
> >> +       if (eth_da->type != RTE_ETH_REPRESENTOR_VF) {
> >Seems like an extra 'if ' condition by mistake? Otherwise there is no
> >diff b/n this 'if' condition and the one few lines above?
>
> Thanks, good catch!
>
> >> +               PMD_DRV_LOG(ERR, "unsupported representor type %d\n",
> >> +                           eth_da->type);
> >> +               return -EINVAL;
> >> +       }
> >>         num_rep = eth_da->nb_representor_ports;
> >>         if (num_rep > BNXT_MAX_VF_REPS) {
> >>                 PMD_DRV_LOG(ERR, "nb_representor_ports = %d > %d MAX VF
> >REPS\n",
> >> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> >> index d041a6bee9..dd085caa93 100644
> >> --- a/drivers/net/enic/enic_ethdev.c
> >> +++ b/drivers/net/enic/enic_ethdev.c
> >> @@ -1303,6 +1303,13 @@ static int eth_enic_pci_probe(struct
> >rte_pci_driver *pci_drv __rte_unused,
> >>                 if (retval)
> >>                         return retval;
> >>         }
> >> +       if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
> >> +               return 0;
> >> +       if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
> >> +               ENICPMD_LOG(ERR, "unsupported representor type: %s\n",
> >> +                           pci_dev->device.devargs->args);
> >> +               return -ENOTSUP;
> >> +       }
> >>         retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
> >>                 sizeof(struct enic),
> >>                 eth_dev_pci_specific_init, pci_dev,
> >> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> >> index f54769c29d..05ed2e1079 100644
> >> --- a/drivers/net/i40e/i40e_ethdev.c
> >> +++ b/drivers/net/i40e/i40e_ethdev.c
> >> @@ -640,6 +640,14 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv
> >__rte_unused,
> >>                         return retval;
> >>         }
> >>
> >> +       if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
> >> +               return 0;
> >> +       if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
> >> +               PMD_DRV_LOG(ERR, "unsupported representor type: %s\n",
> >> +                           pci_dev->device.devargs->args);
> >> +               return -ENOTSUP;
> >> +       }
> >> +
> >>         retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
> >>                 sizeof(struct i40e_adapter),
> >>                 eth_dev_pci_specific_init, pci_dev,
> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> index 9a47a8b262..9ea0139197 100644
> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> @@ -1717,6 +1717,14 @@ eth_ixgbe_pci_probe(struct rte_pci_driver
> >*pci_drv __rte_unused,
> >>         } else
> >>                 memset(&eth_da, 0, sizeof(eth_da));
> >>
> >> +       if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
> >> +               return 0;
> >> +       if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
> >> +               PMD_DRV_LOG(ERR, "unsupported representor type: %s\n",
> >> +                           pci_dev->device.devargs->args);
> >> +               return -ENOTSUP;
> >> +       }
> >> +
> >>         retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
> >>                 sizeof(struct ixgbe_adapter),
> >>                 eth_dev_pci_specific_init, pci_dev,
> >> diff --git a/drivers/net/mlx5/linux/mlx5_os.c
> >b/drivers/net/mlx5/linux/mlx5_os.c
> >> index 6812a1f215..6981ba1f41 100644
> >> --- a/drivers/net/mlx5/linux/mlx5_os.c
> >> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> >> @@ -706,6 +706,17 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
> >>                                 strerror(rte_errno));
> >>                         return NULL;
> >>                 }
> >> +               if (eth_da.type != RTE_ETH_REPRESENTOR_NONE) {
> >> +                       /* Representor not specified. */
> >> +                       rte_errno = EBUSY;
> >> +                       return NULL;
> >> +               }
> >> +               if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
> >> +                       rte_errno = ENOTSUP;
> >> +                       DRV_LOG(ERR, "unsupported representor type: %s",
> >> +                               dpdk_dev->devargs->args);
> >> +                       return NULL;
> >> +               }
> >>                 for (i = 0; i < eth_da.nb_representor_ports; ++i)
> >>                         if (eth_da.representor_ports[i] ==
> >>                             (uint16_t)switch_info->port_name)
> >> diff --git a/lib/librte_ethdev/ethdev_private.c
> >b/lib/librte_ethdev/ethdev_private.c
> >> index 162a502fe7..c219164a4a 100644
> >> --- a/lib/librte_ethdev/ethdev_private.c
> >> +++ b/lib/librte_ethdev/ethdev_private.c
> >> @@ -38,60 +38,13 @@ eth_find_device(const struct rte_eth_dev *start,
> >rte_eth_cmp_t cmp,
> >>         return NULL;
> >>  }
> >>
> >> -int
> >> -rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
> >> -       void *data)
> >> -{
> >> -       char *str_start;
> >> -       int state;
> >> -       int result;
> >> -
> >> -       if (*str != '[')
> >> -               /* Single element, not a list */
> >> -               return callback(str, data);
> >> -
> >> -       /* Sanity check, then strip the brackets */
> >> -       str_start = &str[strlen(str) - 1];
> >> -       if (*str_start != ']') {
> >> -               RTE_LOG(ERR, EAL, "(%s): List does not end with ']'\n", str);
> >> -               return -EINVAL;
> >> -       }
> >> -       str++;
> >> -       *str_start = '\0';
> >> -
> >> -       /* Process list elements */
> >> -       state = 0;
> >> -       while (1) {
> >> -               if (state == 0) {
> >> -                       if (*str == '\0')
> >> -                               break;
> >> -                       if (*str != ',') {
> >> -                               str_start = str;
> >> -                               state = 1;
> >> -                       }
> >> -               } else if (state == 1) {
> >> -                       if (*str == ',' || *str == '\0') {
> >> -                               if (str > str_start) {
> >> -                                       /* Non-empty string fragment */
> >> -                                       *str = '\0';
> >> -                                       result = callback(str_start, data);
> >> -                                       if (result < 0)
> >> -                                               return result;
> >> -                               }
> >> -                               state = 0;
> >> -                       }
> >> -               }
> >> -               str++;
> >> -       }
> >> -       return 0;
> >> -}
> >> -
> >>  static int
> >>  rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
> >>         const uint16_t max_list)
> >>  {
> >>         uint16_t lo, hi, val;
> >>         int result;
> >> +       char *pos = str;
> >>
> >>         result = sscanf(str, "%hu-%hu", &lo, &hi);
> >>         if (result == 1) {
> >> @@ -99,7 +52,7 @@ rte_eth_devargs_process_range(char *str, uint16_t
> >*list, uint16_t *len_list,
> >>                         return -ENOMEM;
> >>                 list[(*len_list)++] = lo;
> >>         } else if (result == 2) {
> >> -               if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS)
> >> +               if (lo >= hi)
> >>                         return -EINVAL;
> >>                 for (val = lo; val <= hi; val++) {
> >>                         if (*len_list >= max_list)
> >> @@ -108,14 +61,52 @@ rte_eth_devargs_process_range(char *str, uint16_t
> >*list, uint16_t *len_list,
> >>                 }
> >>         } else
> >>                 return -EINVAL;
> >> -       return 0;
> >> +       while (*pos != 0 && ((*pos >= '0' && *pos <= '9') || *pos == '-'))
> >> +               pos++;
> >> +       return pos - str;
> >>  }
> >>
> >> +static int
> >> +rte_eth_devargs_process_list(char *str, uint16_t *list, uint16_t *len_list,
> >> +       const uint16_t max_list)
> >> +{
> >> +       char *pos = str;
> >> +       int ret;
> >> +
> >> +       if (*pos == '[')
> >> +               pos++;
> >> +       while (1) {
> >> +               ret = rte_eth_devargs_process_range(pos, list, len_list,
> >> +                                                   max_list);
> >> +               if (ret < 0)
> >> +                       return ret;
> >> +               pos += ret;
> >> +               if (*pos != ',') /* end of list */
> >> +                       break;
> >> +               pos++;
> >> +       }
> >> +       if (*str == '[' && *pos != ']')
> >> +               return -EINVAL;
> >> +       if (*pos == ']')
> >> +               pos++;
> >> +       return pos - str;
> >> +}
> >> +
> >> +/*
> >> + * representor format:
> >> + *   #: range or single number of VF representor - legacy
> >> + */
> >>  int
> >>  rte_eth_devargs_parse_representor_ports(char *str, void *data)
> >>  {
> >>         struct rte_eth_devargs *eth_da = data;
> >> +       int ret;
> >>
> >> -       return rte_eth_devargs_process_range(str, eth_da->representor_ports,
> >> +       /* Number # alone implies VF */
> >> +       eth_da->type = RTE_ETH_REPRESENTOR_VF;
> >> +       ret = rte_eth_devargs_process_list(str, eth_da->representor_ports,
> >>                 &eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
> >> +       if (ret < 0)
> >> +               RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
> >> +       return ret < 0 ? ret : 0;
> >>  }
> >> diff --git a/lib/librte_ethdev/ethdev_private.h
> >b/lib/librte_ethdev/ethdev_private.h
> >> index 905a45c337..220ddd4408 100644
> >> --- a/lib/librte_ethdev/ethdev_private.h
> >> +++ b/lib/librte_ethdev/ethdev_private.h
> >> @@ -26,9 +26,6 @@ eth_find_device(const struct rte_eth_dev *_start,
> >rte_eth_cmp_t cmp,
> >>                 const void *data);
> >>
> >>  /* Parse devargs value for representor parameter. */
> >> -typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
> >> -int rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t
> >callback,
> >> -       void *data);
> >>  int rte_eth_devargs_parse_representor_ports(char *str, void *data);
> >>
> >>  #ifdef __cplusplus
> >> diff --git a/lib/librte_ethdev/rte_class_eth.c
> >b/lib/librte_ethdev/rte_class_eth.c
> >> index 6338355e25..efe6149df5 100644
> >> --- a/lib/librte_ethdev/rte_class_eth.c
> >> +++ b/lib/librte_ethdev/rte_class_eth.c
> >> @@ -77,9 +77,7 @@ eth_representor_cmp(const char *key __rte_unused,
> >>         if (values == NULL)
> >>                 return -1;
> >>         memset(&representors, 0, sizeof(representors));
> >> -       ret = rte_eth_devargs_parse_list(values,
> >> -                       rte_eth_devargs_parse_representor_ports,
> >> -                       &representors);
> >> +       ret = rte_eth_devargs_parse_representor_ports(values, &representors);
> >>         free(values);
> >>         if (ret != 0)
> >>                 return -1; /* invalid devargs value */
> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> >> index 17ddacc78d..2ac51ac149 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.c
> >> +++ b/lib/librte_ethdev/rte_ethdev.c
> >> @@ -5542,9 +5542,8 @@ rte_eth_devargs_parse(const char *dargs, struct
> >rte_eth_devargs *eth_da)
> >>         for (i = 0; i < args.count; i++) {
> >>                 pair = &args.pairs[i];
> >>                 if (strcmp("representor", pair->key) == 0) {
> >> -                       result = rte_eth_devargs_parse_list(pair->value,
> >> -                               rte_eth_devargs_parse_representor_ports,
> >> -                               eth_da);
> >> +                       result = rte_eth_devargs_parse_representor_ports(
> >> +                                       pair->value, eth_da);
> >>                         if (result < 0)
> >>                                 goto parse_cleanup;
> >>                 }
> >> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
> >b/lib/librte_ethdev/rte_ethdev_driver.h
> >> index 0eacfd8425..b66a955b18 100644
> >> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> >> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> >> @@ -1193,6 +1193,12 @@ __rte_internal
> >>  int
> >>  rte_eth_switch_domain_free(uint16_t domain_id);
> >>
> >> +/** Ethernet device representor type */
> >> +enum rte_eth_representor_type {
> >> +       RTE_ETH_REPRESENTOR_NONE, /* not a representor */
> >> +       RTE_ETH_REPRESENTOR_VF,   /* representor of VF */
> >> +};
> >> +
> >>  /** Generic Ethernet device arguments  */
> >>  struct rte_eth_devargs {
> >>         uint16_t ports[RTE_MAX_ETHPORTS];
> >> @@ -1203,6 +1209,7 @@ struct rte_eth_devargs {
> >>         /** representor port/s identifier to enable on device */
> >>         uint16_t nb_representor_ports;
> >>         /** number of ports in representor port field */
> >> +       enum rte_eth_representor_type type; /* type of representor */
> >>  };
> >>
> >>  /**
> >> --
> >> 2.25.1
> >>
> >
> >--
> >This electronic communication and the information and any files transmitted
> >with it, or attached to it, are confidential and are intended solely for
> >the use of the individual or entity to whom it is addressed and may contain
> >information that is confidential, legally privileged, protected by privacy
> >laws, or otherwise restricted from disclosure to anyone else. If you are
> >not the intended recipient or the person responsible for delivering the
> >e-mail to the intended recipient, you are hereby notified that any use,
> >copying, distributing, dissemination, forwarding, printing, or copying of
> >this e-mail is strictly prohibited. If you received this e-mail in error,
> >please return the e-mail to the sender, delete it from your computer, and
> >destroy any printed copy of it.

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.