[ovs-dev,3/3] netdev-dpdk: Use uint8_t for port_id.

On 5/12/17, 8:04 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:

    On 05.04.2017 22:34, Darrell Ball wrote:
    > 

    > 

    > On 4/3/17, 8:04 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ilya Maximets" <ovs-dev-bounces@openvswitch.org on behalf of i.maximets@samsung.com> wrote:

    > 

    >     Currently, signed integer is used for 'port_id' variable and

    >     '-1' as identifier of bad or uninitialized 'port_id'.

    >     

    >     This inconsistent with dpdk library and, also, in few cases,

    >     leads to passing '-1' to dpdk functions where uint8_t expected.

    >     

    >     Such behaviour doesn't produce any issues, but it's better to

    >     use same type as in dpdk library for consistency.

    >     

    >     Also, magic number '-1' replaced with DPDK_ETH_PORT_ID_INVALID

    >     macro.

    >     

    >     Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

    >     ---

    >      lib/netdev-dpdk.c | 61 +++++++++++++++++++++++++++++++------------------------

    >      1 file changed, 35 insertions(+), 26 deletions(-)

    >     

    >     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c

    >     index 658a454..216ced8 100644

    >     --- a/lib/netdev-dpdk.c

    >     +++ b/lib/netdev-dpdk.c

    >     @@ -140,6 +140,8 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))

    >      #define OVS_VHOST_QUEUE_DISABLED    (-2) /* Queue was disabled by guest and not

    >                                                * yet mapped to another queue. */

    >      

    >     +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS

    >     +

    >      #define VHOST_ENQ_RETRY_NUM 8

    >      #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)

    >      

    >     @@ -309,7 +311,7 @@ struct dpdk_ring {

    >          struct rte_ring *cring_tx;

    >          struct rte_ring *cring_rx;

    >          unsigned int user_port_id; /* User given port no, parsed from port name */

    >     -    int eth_port_id; /* ethernet device port id */

    >     +    uint8_t eth_port_id; /* ethernet device port id */

    > 

    > The rte does not have a typedef for port_id.

    > One optional change is for OVS to have a typedef for this external type.

    > /* The dpdk rte uses uint8_t for port_id. */

    > typedef uint8_t rte_port_id;

    
    I don't think that it is a good change to do because we will have to
    create additional typedef at least for PRIu8. This will look ugly.

No, see my following diff

    Also, if DPDK someday will create typedef with different name
    we will have typedef of the typedef?

I don’t follow this comment; see the diff below.

The reasons for the typedef here are:
1) Easier to maintain if the size of type changes in future
2) Serves as documentation that the origin of the type is the rte library and
and originates externally to ovs

Here is a few examples of typedefs in the OVS code base:

dpif-netdev.c:5587:    typedef uint32_t map_type;
stp.h:41:typedef uint64_t stp_identifier;
timeval.c:45:typedef unsigned int clockid_t;


dball@ubuntu:~/ovs$ git diff
(END)





    
    > 

    >          struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);

    >      };

    >      

    >     @@ -325,7 +327,7 @@ enum dpdk_hw_ol_features {

    >      

    >      struct netdev_dpdk {

    >          struct netdev up;

    >     -    int port_id;

    >     +    uint8_t port_id;

    >          int max_packet_len;

    >          enum dpdk_dev_type type;

    >      

    >     @@ -402,7 +404,7 @@ struct netdev_dpdk {

    >      

    >      struct netdev_rxq_dpdk {

    >          struct netdev_rxq up;

    >     -    int port_id;

    >     +    uint8_t port_id;

    >      };

    >      

    >      static int netdev_dpdk_class_init(void);

    >     @@ -602,12 +604,12 @@ check_link_status(struct netdev_dpdk *dev)

    >              dev->link_reset_cnt++;

    >              dev->link = link;

    >              if (dev->link.link_status) {

    >     -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",

    >     +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",

    >                              dev->port_id, (unsigned) dev->link.link_speed,

    >                              (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?

    >                               ("full-duplex") : ("half-duplex"));

    >              } else {

    >     -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);

    >     +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);

    >              }

    >          }

    >      }

    >     @@ -725,8 +727,8 @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)

    >          if (rx_csum_ol_flag &&

    >              (info.rx_offload_capa & rx_chksm_offload_capa) !=

    >               rx_chksm_offload_capa) {

    >     -        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %d",

    >     -                   dev->port_id);

    >     +        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %"PRIu8,

    >     +                       dev->port_id);

    >              dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;

    >              return;

    >          }

    >     @@ -737,7 +739,8 @@ static void

    >      dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)

    >      {

    >          if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {

    >     -        VLOG_WARN("Failed to enable flow control on device %d", dev->port_id);

    >     +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,

    >     +                  dev->port_id);

    >          }

    >      }

    >      

    >     @@ -775,7 +778,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >      

    >          memset(&eth_addr, 0x0, sizeof(eth_addr));

    >          rte_eth_macaddr_get(dev->port_id, &eth_addr);

    >     -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,

    >     +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,

    >                          dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));

    >      

    >          memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);

    >     @@ -787,7 +790,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >          /* Get the Flow control configuration for DPDK-ETH */

    >          diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);

    >          if (diag) {

    >     -        VLOG_DBG("cannot get flow control parameters on port=%d, err=%d",

    >     +        VLOG_DBG("cannot get flow control parameters on port=%"PRIu8", err=%d",

    >                       dev->port_id, diag);

    >          }

    >      

    >     @@ -832,7 +835,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)

    >      }

    >      

    >      static int

    >     -common_construct(struct netdev *netdev, unsigned int port_no,

    >     +common_construct(struct netdev *netdev, uint8_t port_no,

    >                       enum dpdk_dev_type type, int socket_id)

    >          OVS_REQUIRES(dpdk_mutex)

    >      {

    >     @@ -917,7 +920,8 @@ vhost_common_construct(struct netdev *netdev)

    >              return ENOMEM;

    >          }

    >      

    >     -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);

    >     +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    >     +                            DPDK_DEV_VHOST, socket_id);

    >      }

    >      

    >      static int

    >     @@ -977,7 +981,8 @@ netdev_dpdk_construct(struct netdev *netdev)

    >          int err;

    >      

    >          ovs_mutex_lock(&dpdk_mutex);

    >     -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);

    >     +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    >     +                           DPDK_DEV_ETH, SOCKET0);

    >          ovs_mutex_unlock(&dpdk_mutex);

    >          return err;

    >      }

    >     @@ -1111,7 +1116,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)

    >      }

    >      

    >      static struct netdev_dpdk *

    >     -netdev_dpdk_lookup_by_port_id(int port_id)

    >     +netdev_dpdk_lookup_by_port_id(uint8_t port_id)

    >          OVS_REQUIRES(dpdk_mutex)

    >      {

    >          struct netdev_dpdk *dev;

    >     @@ -1125,10 +1130,11 @@ netdev_dpdk_lookup_by_port_id(int port_id)

    >          return NULL;

    >      }

    >      

    >     -static int

    >     -netdev_dpdk_process_devargs(const char *devargs, char **errp)

    >     +static uint8_t

    >     +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,

    >     +                            const char *devargs, char **errp)

    >      {

    >     -    uint8_t new_port_id = UINT8_MAX;

    >     +    uint8_t new_port_id = DPDK_ETH_PORT_ID_INVALID;

    >          char *ind, *name = xstrdup(devargs);

    >      

    >          /* Get the name from the comma separated list of arguments. */

    >     @@ -1148,7 +1154,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)

    >              } else {

    >                  /* Attach unsuccessful */

    >                  VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);

    >     -            return -1;

    >     +            new_port_id = DPDK_ETH_PORT_ID_INVALID;

    >              }

    >          }

    >      

    >     @@ -1229,7 +1235,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,

    >               * is valid */

    >              if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)

    >                     && rte_eth_dev_is_valid_port(dev->port_id))) {

    >     -            int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);

    >     +            uint8_t new_port_id = netdev_dpdk_process_devargs(dev, new_devargs,

    >     +                                                              errp);

    >                  if (!rte_eth_dev_is_valid_port(new_port_id)) {

    >                      err = EINVAL;

    >                  } else if (new_port_id == dev->port_id) {

    >     @@ -1237,6 +1244,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,

    >                      err = 0;

    >                  } else {

    >                      struct netdev_dpdk *dup_dev;

    >     +

    >                      dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);

    >                      if (dup_dev) {

    >                          VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "

    >     @@ -1246,6 +1254,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,

    >                          err = EADDRINUSE;

    >                      } else {

    >                          int sid = rte_eth_dev_socket_id(new_port_id);

    >     +

    >                          dev->requested_socket_id = sid < 0 ? SOCKET0 : sid;

    >                          dev->devargs = xstrdup(new_devargs);

    >                          dev->port_id = new_port_id;

    >     @@ -2051,7 +2060,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >          int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;

    >      

    >          if (rte_eth_stats_get(dev->port_id, &rte_stats)) {

    >     -        VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);

    >     +        VLOG_ERR("Can't get ETH statistics for port: %"PRIu8, dev->port_id);

    >              ovs_mutex_unlock(&dev->mutex);

    >              return EPROTO;

    >          }

    >     @@ -2059,7 +2068,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >          /* Get length of statistics */

    >          rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);

    >          if (rte_xstats_len < 0) {

    >     -        VLOG_WARN("Cannot get XSTATS values for port: %i", dev->port_id);

    >     +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);

    >              goto out;

    >          }

    >          /* Reserve memory for xstats names and values */

    >     @@ -2071,7 +2080,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >                                                        rte_xstats_names,

    >                                                        rte_xstats_len);

    >          if (rte_xstats_new_len != rte_xstats_len) {

    >     -        VLOG_WARN("Cannot get XSTATS names for port: %i.", dev->port_id);

    >     +        VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id);

    >              goto out;

    >          }

    >          /* Retreive xstats values */

    >     @@ -2082,7 +2091,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >              netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,

    >                                         rte_xstats_len);

    >          } else {

    >     -        VLOG_WARN("Cannot get XSTATS values for port: %i.", dev->port_id);

    >     +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);

    >          }

    >      

    >      out:

    >     @@ -2759,7 +2768,7 @@ netdev_dpdk_vhost_class_init(void)

    >      

    >      static int

    >      dpdk_ring_create(const char dev_name[], unsigned int port_no,

    >     -                 unsigned int *eth_port_id)

    >     +                 uint8_t *eth_port_id)

    >      {

    >          struct dpdk_ring *ring_pair;

    >          char *ring_name;

    >     @@ -2811,7 +2820,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no,

    >      }

    >      

    >      static int

    >     -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)

    >     +dpdk_ring_open(const char dev_name[], uint8_t *eth_port_id)

    >          OVS_REQUIRES(dpdk_mutex)

    >      {

    >          struct dpdk_ring *ring_pair;

    >     @@ -2860,7 +2869,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,

    >      static int

    >      netdev_dpdk_ring_construct(struct netdev *netdev)

    >      {

    >     -    unsigned int port_no = 0;

    >     +    uint8_t port_no = 0;

    >          int err = 0;

    >      

    >          ovs_mutex_lock(&dpdk_mutex);

    >     -- 

    >     2.7.4

    >     

    >     _______________________________________________

    >     dev mailing list

    >     dev@openvswitch.org

    >     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=CsrWZo9SNec-_DnojNdU2sRwAPpUXsyy5cTNk5hkhaQ&s=6OANH43fV6QTakoajvWrqyZoezwlGPQF4pLjlhA50ss&e= 

    >     

    >

I guess, we need some more opinions about this.

My comments inline.

Best regards, Ilya Maximets.

On 13.05.2017 07:00, Darrell Ball wrote:
> 
> 
> On 5/12/17, 8:04 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:
> 
>     On 05.04.2017 22:34, Darrell Ball wrote:
>     > 
>     > 
>     > On 4/3/17, 8:04 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ilya Maximets" <ovs-dev-bounces@openvswitch.org on behalf of i.maximets@samsung.com> wrote:
>     > 
>     >     Currently, signed integer is used for 'port_id' variable and
>     >     '-1' as identifier of bad or uninitialized 'port_id'.
>     >     
>     >     This inconsistent with dpdk library and, also, in few cases,
>     >     leads to passing '-1' to dpdk functions where uint8_t expected.
>     >     
>     >     Such behaviour doesn't produce any issues, but it's better to
>     >     use same type as in dpdk library for consistency.
>     >     
>     >     Also, magic number '-1' replaced with DPDK_ETH_PORT_ID_INVALID
>     >     macro.
>     >     
>     >     Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>     >     ---
>     >      lib/netdev-dpdk.c | 61 +++++++++++++++++++++++++++++++------------------------
>     >      1 file changed, 35 insertions(+), 26 deletions(-)
>     >     
>     >     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>     >     index 658a454..216ced8 100644
>     >     --- a/lib/netdev-dpdk.c
>     >     +++ b/lib/netdev-dpdk.c
>     >     @@ -140,6 +140,8 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>     >      #define OVS_VHOST_QUEUE_DISABLED    (-2) /* Queue was disabled by guest and not
>     >                                                * yet mapped to another queue. */
>     >      
>     >     +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS
>     >     +
>     >      #define VHOST_ENQ_RETRY_NUM 8
>     >      #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>     >      
>     >     @@ -309,7 +311,7 @@ struct dpdk_ring {
>     >          struct rte_ring *cring_tx;
>     >          struct rte_ring *cring_rx;
>     >          unsigned int user_port_id; /* User given port no, parsed from port name */
>     >     -    int eth_port_id; /* ethernet device port id */
>     >     +    uint8_t eth_port_id; /* ethernet device port id */
>     > 
>     > The rte does not have a typedef for port_id.
>     > One optional change is for OVS to have a typedef for this external type.
>     > /* The dpdk rte uses uint8_t for port_id. */
>     > typedef uint8_t rte_port_id;
>     
>     I don't think that it is a good change to do because we will have to
>     create additional typedef at least for PRIu8. This will look ugly.
> 
> No, see my following diff
> 
>     Also, if DPDK someday will create typedef with different name
>     we will have typedef of the typedef?
> 
> I don’t follow this comment; see the diff below.
> 
> The reasons for the typedef here are:
> 1) Easier to maintain if the size of type changes in future

In case of future type change we will have to change all the "PRIu8"
format strings to something else. This is the half of all the changes.
So, maintainability is in question.

> 2) Serves as documentation that the origin of the type is the rte library and
> and originates externally to ovs

IMHO, using 'rte_' prefix for something defined not inside DPDK is a
bad style and may be misleading. We should remember that this type
defined locally in OVS.

> 
> Here is a few examples of typedefs in the OVS code base:
> 
> dpif-netdev.c:5587:    typedef uint32_t map_type;
> stp.h:41:typedef uint64_t stp_identifier;
> timeval.c:45:typedef unsigned int clockid_t;
> 
> 
> dball@ubuntu:~/ovs$ git diff
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 609b8da..6667274 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -143,6 +143,9 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>  #define VHOST_ENQ_RETRY_NUM 8
>  #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>  
> +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS
> +typedef uint8_t rte_port_id;
> +
>  static const struct rte_eth_conf port_conf = {
>      .rxmode = {
>          .mq_mode = ETH_MQ_RX_RSS,
> @@ -309,7 +312,7 @@ struct dpdk_ring {
>      struct rte_ring *cring_tx;
>      struct rte_ring *cring_rx;
>      unsigned int user_port_id; /* User given port no, parsed from port name */
> -    int eth_port_id; /* ethernet device port id */
> +    rte_port_id eth_port_id; /* ethernet device port id */
>      struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>  };
>  
> @@ -325,7 +328,7 @@ enum dpdk_hw_ol_features {
>  
>  struct netdev_dpdk {
>      struct netdev up;
> -    int port_id;
> +    rte_port_id port_id;
>      int max_packet_len;
>      enum dpdk_dev_type type;
>  
> @@ -399,7 +402,7 @@ struct netdev_dpdk {
>  
>  struct netdev_rxq_dpdk {
>      struct netdev_rxq up;
> -    int port_id;
> +    rte_port_id port_id;
>  };
>  
>  static int netdev_dpdk_class_init(void);
> @@ -600,12 +603,12 @@ check_link_status(struct netdev_dpdk *dev)
>          dev->link_reset_cnt++;
>          dev->link = link;
>          if (dev->link.link_status) {
> -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
> +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
>                          dev->port_id, (unsigned) dev->link.link_speed,
>                          (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>                           ("full-duplex") : ("half-duplex"));
>          } else {
> -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
> +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
>          }
>      }
>  }
> @@ -723,7 +726,7 @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)
>      if (rx_csum_ol_flag &&
>          (info.rx_offload_capa & rx_chksm_offload_capa) !=
>           rx_chksm_offload_capa) {
> -        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %d",
> +        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %"PRIu8,
>                     dev->port_id);
>          dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>          return;
> @@ -735,7 +738,8 @@ static void
>  dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)
>  {
>      if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
> -        VLOG_WARN("Failed to enable flow control on device %d", dev->port_id);
> +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,
> +                  dev->port_id);
>      }
>  }
>  
> @@ -773,7 +777,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>  
>      memset(&eth_addr, 0x0, sizeof(eth_addr));
>      rte_eth_macaddr_get(dev->port_id, &eth_addr);
> -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
> +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
>                      dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>  
>      memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
> @@ -785,7 +789,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>      /* Get the Flow control configuration for DPDK-ETH */
>      diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
>      if (diag) {
> -        VLOG_DBG("cannot get flow control parameters on port=%d, err=%d",
> +        VLOG_DBG("cannot get flow control parameters on port=%"PRIu8", err=%d",
>                   dev->port_id, diag);
>      }
>  
> @@ -830,7 +834,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
>  }
>  
>  static int
> -common_construct(struct netdev *netdev, unsigned int port_no,
> +common_construct(struct netdev *netdev, rte_port_id port_no,
>                   enum dpdk_dev_type type, int socket_id)
>      OVS_REQUIRES(dpdk_mutex)
>  {
> @@ -914,7 +918,8 @@ vhost_common_construct(struct netdev *netdev)
>          return ENOMEM;
>      }
>  
> -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);
> +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
> +                            DPDK_DEV_VHOST, socket_id);
>  }
>  
>  static int
> @@ -974,7 +979,8 @@ netdev_dpdk_construct(struct netdev *netdev)
>      int err;
>  
>      ovs_mutex_lock(&dpdk_mutex);
> -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
> +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
> +                           DPDK_DEV_ETH, SOCKET0);
>      ovs_mutex_unlock(&dpdk_mutex);
>      return err;
>  }
> @@ -1097,7 +1103,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)
>  }
>  
>  static struct netdev_dpdk *
> -netdev_dpdk_lookup_by_port_id(int port_id)
> +netdev_dpdk_lookup_by_port_id(rte_port_id port_id)
>      OVS_REQUIRES(dpdk_mutex)
>  {
>      struct netdev_dpdk *dev;
> @@ -1111,7 +1117,7 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>      return NULL;
>  }
>  
> -static int
> +static rte_port_id
>  netdev_dpdk_process_devargs(const char *devargs, char **errp)
>  {
>      uint8_t new_port_id = UINT8_MAX;
> @@ -1126,7 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
>          } else {
>              /* Attach unsuccessful */
>              VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
> -            return -1;
> +            new_port_id = DPDK_ETH_PORT_ID_INVALID;
>          }
>      }
>  
> @@ -1206,7 +1212,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>           * is valid */
>          if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
>                 && rte_eth_dev_is_valid_port(dev->port_id))) {
> -            int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);
> +            rte_port_id new_port_id = netdev_dpdk_process_devargs(new_devargs,
> +                                                                  errp);
>              if (!rte_eth_dev_is_valid_port(new_port_id)) {
>                  err = EINVAL;
>              } else if (new_port_id == dev->port_id) {
> @@ -2028,7 +2035,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>      int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
>  
>      if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
> -        VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);
> +        VLOG_ERR("Can't get ETH statistics for port: %"PRIu8, dev->port_id);
>          ovs_mutex_unlock(&dev->mutex);
>          return EPROTO;
>      }
> @@ -2036,7 +2043,8 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>      /* Get length of statistics */
>      rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
>      if (rte_xstats_len < 0) {
> -        VLOG_WARN("Cannot get XSTATS values for port: %i", dev->port_id);
> +        VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id);
> +
>          goto out;
>      }
>      /* Reserve memory for xstats names and values */
> @@ -2059,7 +2067,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>          netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
>                                     rte_xstats_len);
>      } else {
> -        VLOG_WARN("Cannot get XSTATS values for port: %i.", dev->port_id);
> +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);
>      }
>  
>  out:
> @@ -2785,7 +2793,7 @@ netdev_dpdk_vhost_class_init(void)
>  
>  static int
>  dpdk_ring_create(const char dev_name[], unsigned int port_no,
> -                 unsigned int *eth_port_id)
> +                 rte_port_id *eth_port_id)
>  {
>      struct dpdk_ring *ring_pair;
>      char *ring_name;
> @@ -2837,7 +2845,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no,
>  }
>  
>  static int
> -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)
> +dpdk_ring_open(const char dev_name[], rte_port_id *eth_port_id)
>      OVS_REQUIRES(dpdk_mutex)
>  {
>      struct dpdk_ring *ring_pair;
> @@ -2886,7 +2894,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,
>  static int
>  netdev_dpdk_ring_construct(struct netdev *netdev)
>  {
> -    unsigned int port_no = 0;
> +    rte_port_id port_no = 0;
>      int err = 0;
>  
>      ovs_mutex_lock(&dpdk_mutex);
> (END)
> 
> 
> 
> 
> 
>     
>     > 
>     >          struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>     >      };
>     >      
>     >     @@ -325,7 +327,7 @@ enum dpdk_hw_ol_features {
>     >      
>     >      struct netdev_dpdk {
>     >          struct netdev up;
>     >     -    int port_id;
>     >     +    uint8_t port_id;
>     >          int max_packet_len;
>     >          enum dpdk_dev_type type;
>     >      
>     >     @@ -402,7 +404,7 @@ struct netdev_dpdk {
>     >      
>     >      struct netdev_rxq_dpdk {
>     >          struct netdev_rxq up;
>     >     -    int port_id;
>     >     +    uint8_t port_id;
>     >      };
>     >      
>     >      static int netdev_dpdk_class_init(void);
>     >     @@ -602,12 +604,12 @@ check_link_status(struct netdev_dpdk *dev)
>     >              dev->link_reset_cnt++;
>     >              dev->link = link;
>     >              if (dev->link.link_status) {
>     >     -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
>     >     +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
>     >                              dev->port_id, (unsigned) dev->link.link_speed,
>     >                              (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>     >                               ("full-duplex") : ("half-duplex"));
>     >              } else {
>     >     -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
>     >     +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
>     >              }
>     >          }
>     >      }
>     >     @@ -725,8 +727,8 @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)
>     >          if (rx_csum_ol_flag &&
>     >              (info.rx_offload_capa & rx_chksm_offload_capa) !=
>     >               rx_chksm_offload_capa) {
>     >     -        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %d",
>     >     -                   dev->port_id);
>     >     +        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %"PRIu8,
>     >     +                       dev->port_id);
>     >              dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>     >              return;
>     >          }
>     >     @@ -737,7 +739,8 @@ static void
>     >      dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)
>     >      {
>     >          if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
>     >     -        VLOG_WARN("Failed to enable flow control on device %d", dev->port_id);
>     >     +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,
>     >     +                  dev->port_id);
>     >          }
>     >      }
>     >      
>     >     @@ -775,7 +778,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>     >      
>     >          memset(&eth_addr, 0x0, sizeof(eth_addr));
>     >          rte_eth_macaddr_get(dev->port_id, &eth_addr);
>     >     -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
>     >     +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
>     >                          dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>     >      
>     >          memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
>     >     @@ -787,7 +790,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>     >          /* Get the Flow control configuration for DPDK-ETH */
>     >          diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
>     >          if (diag) {
>     >     -        VLOG_DBG("cannot get flow control parameters on port=%d, err=%d",
>     >     +        VLOG_DBG("cannot get flow control parameters on port=%"PRIu8", err=%d",
>     >                       dev->port_id, diag);
>     >          }
>     >      
>     >     @@ -832,7 +835,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
>     >      }
>     >      
>     >      static int
>     >     -common_construct(struct netdev *netdev, unsigned int port_no,
>     >     +common_construct(struct netdev *netdev, uint8_t port_no,
>     >                       enum dpdk_dev_type type, int socket_id)
>     >          OVS_REQUIRES(dpdk_mutex)
>     >      {
>     >     @@ -917,7 +920,8 @@ vhost_common_construct(struct netdev *netdev)
>     >              return ENOMEM;
>     >          }
>     >      
>     >     -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);
>     >     +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>     >     +                            DPDK_DEV_VHOST, socket_id);
>     >      }
>     >      
>     >      static int
>     >     @@ -977,7 +981,8 @@ netdev_dpdk_construct(struct netdev *netdev)
>     >          int err;
>     >      
>     >          ovs_mutex_lock(&dpdk_mutex);
>     >     -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
>     >     +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>     >     +                           DPDK_DEV_ETH, SOCKET0);
>     >          ovs_mutex_unlock(&dpdk_mutex);
>     >          return err;
>     >      }
>     >     @@ -1111,7 +1116,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)
>     >      }
>     >      
>     >      static struct netdev_dpdk *
>     >     -netdev_dpdk_lookup_by_port_id(int port_id)
>     >     +netdev_dpdk_lookup_by_port_id(uint8_t port_id)
>     >          OVS_REQUIRES(dpdk_mutex)
>     >      {
>     >          struct netdev_dpdk *dev;
>     >     @@ -1125,10 +1130,11 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>     >          return NULL;
>     >      }
>     >      
>     >     -static int
>     >     -netdev_dpdk_process_devargs(const char *devargs, char **errp)
>     >     +static uint8_t
>     >     +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
>     >     +                            const char *devargs, char **errp)
>     >      {
>     >     -    uint8_t new_port_id = UINT8_MAX;
>     >     +    uint8_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
>     >          char *ind, *name = xstrdup(devargs);
>     >      
>     >          /* Get the name from the comma separated list of arguments. */
>     >     @@ -1148,7 +1154,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
>     >              } else {
>     >                  /* Attach unsuccessful */
>     >                  VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
>     >     -            return -1;
>     >     +            new_port_id = DPDK_ETH_PORT_ID_INVALID;
>     >              }
>     >          }
>     >      
>     >     @@ -1229,7 +1235,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>     >               * is valid */
>     >              if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
>     >                     && rte_eth_dev_is_valid_port(dev->port_id))) {
>     >     -            int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);
>     >     +            uint8_t new_port_id = netdev_dpdk_process_devargs(dev, new_devargs,
>     >     +                                                              errp);
>     >                  if (!rte_eth_dev_is_valid_port(new_port_id)) {
>     >                      err = EINVAL;
>     >                  } else if (new_port_id == dev->port_id) {
>     >     @@ -1237,6 +1244,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>     >                      err = 0;
>     >                  } else {
>     >                      struct netdev_dpdk *dup_dev;
>     >     +
>     >                      dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
>     >                      if (dup_dev) {
>     >                          VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "
>     >     @@ -1246,6 +1254,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>     >                          err = EADDRINUSE;
>     >                      } else {
>     >                          int sid = rte_eth_dev_socket_id(new_port_id);
>     >     +
>     >                          dev->requested_socket_id = sid < 0 ? SOCKET0 : sid;
>     >                          dev->devargs = xstrdup(new_devargs);
>     >                          dev->port_id = new_port_id;
>     >     @@ -2051,7 +2060,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >          int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
>     >      
>     >          if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
>     >     -        VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);
>     >     +        VLOG_ERR("Can't get ETH statistics for port: %"PRIu8, dev->port_id);
>     >              ovs_mutex_unlock(&dev->mutex);
>     >              return EPROTO;
>     >          }
>     >     @@ -2059,7 +2068,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >          /* Get length of statistics */
>     >          rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
>     >          if (rte_xstats_len < 0) {
>     >     -        VLOG_WARN("Cannot get XSTATS values for port: %i", dev->port_id);
>     >     +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);
>     >              goto out;
>     >          }
>     >          /* Reserve memory for xstats names and values */
>     >     @@ -2071,7 +2080,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >                                                        rte_xstats_names,
>     >                                                        rte_xstats_len);
>     >          if (rte_xstats_new_len != rte_xstats_len) {
>     >     -        VLOG_WARN("Cannot get XSTATS names for port: %i.", dev->port_id);
>     >     +        VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id);
>     >              goto out;
>     >          }
>     >          /* Retreive xstats values */
>     >     @@ -2082,7 +2091,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >              netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
>     >                                         rte_xstats_len);
>     >          } else {
>     >     -        VLOG_WARN("Cannot get XSTATS values for port: %i.", dev->port_id);
>     >     +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);
>     >          }
>     >      
>     >      out:
>     >     @@ -2759,7 +2768,7 @@ netdev_dpdk_vhost_class_init(void)
>     >      
>     >      static int
>     >      dpdk_ring_create(const char dev_name[], unsigned int port_no,
>     >     -                 unsigned int *eth_port_id)
>     >     +                 uint8_t *eth_port_id)
>     >      {
>     >          struct dpdk_ring *ring_pair;
>     >          char *ring_name;
>     >     @@ -2811,7 +2820,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no,
>     >      }
>     >      
>     >      static int
>     >     -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)
>     >     +dpdk_ring_open(const char dev_name[], uint8_t *eth_port_id)
>     >          OVS_REQUIRES(dpdk_mutex)
>     >      {
>     >          struct dpdk_ring *ring_pair;
>     >     @@ -2860,7 +2869,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,
>     >      static int
>     >      netdev_dpdk_ring_construct(struct netdev *netdev)
>     >      {
>     >     -    unsigned int port_no = 0;
>     >     +    uint8_t port_no = 0;
>     >          int err = 0;
>     >      
>     >          ovs_mutex_lock(&dpdk_mutex);
>     >     -- 
>     >     2.7.4
>     >     
>     >     _______________________________________________
>     >     dev mailing list
>     >     dev@openvswitch.org
>     >     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=CsrWZo9SNec-_DnojNdU2sRwAPpUXsyy5cTNk5hkhaQ&s=6OANH43fV6QTakoajvWrqyZoezwlGPQF4pLjlhA50ss&e= 
>     >     
>     > 
>     
>

On 5/17/17, 7:59 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:

    I guess, we need some more opinions about this.
    
    My comments inline.
    
    Best regards, Ilya Maximets.
    
    On 13.05.2017 07:00, Darrell Ball wrote:
    > 

    > 

    > On 5/12/17, 8:04 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:

    > 

    >     On 05.04.2017 22:34, Darrell Ball wrote:

    >     > 

    >     > 

    >     > On 4/3/17, 8:04 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ilya Maximets" <ovs-dev-bounces@openvswitch.org on behalf of i.maximets@samsung.com> wrote:

    >     > 

    >     >     Currently, signed integer is used for 'port_id' variable and

    >     >     '-1' as identifier of bad or uninitialized 'port_id'.

    >     >     

    >     >     This inconsistent with dpdk library and, also, in few cases,

    >     >     leads to passing '-1' to dpdk functions where uint8_t expected.

    >     >     

    >     >     Such behaviour doesn't produce any issues, but it's better to

    >     >     use same type as in dpdk library for consistency.

    >     >     

    >     >     Also, magic number '-1' replaced with DPDK_ETH_PORT_ID_INVALID

    >     >     macro.

    >     >     

    >     >     Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

    >     >     ---

    >     >      lib/netdev-dpdk.c | 61 +++++++++++++++++++++++++++++++------------------------

    >     >      1 file changed, 35 insertions(+), 26 deletions(-)

    >     >     

    >     >     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c

    >     >     index 658a454..216ced8 100644

    >     >     --- a/lib/netdev-dpdk.c

    >     >     +++ b/lib/netdev-dpdk.c

    >     >     @@ -140,6 +140,8 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))

    >     >      #define OVS_VHOST_QUEUE_DISABLED    (-2) /* Queue was disabled by guest and not

    >     >                                                * yet mapped to another queue. */

    >     >      

    >     >     +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS

    >     >     +

    >     >      #define VHOST_ENQ_RETRY_NUM 8

    >     >      #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)

    >     >      

    >     >     @@ -309,7 +311,7 @@ struct dpdk_ring {

    >     >          struct rte_ring *cring_tx;

    >     >          struct rte_ring *cring_rx;

    >     >          unsigned int user_port_id; /* User given port no, parsed from port name */

    >     >     -    int eth_port_id; /* ethernet device port id */

    >     >     +    uint8_t eth_port_id; /* ethernet device port id */

    >     > 

    >     > The rte does not have a typedef for port_id.

    >     > One optional change is for OVS to have a typedef for this external type.

    >     > /* The dpdk rte uses uint8_t for port_id. */

    >     > typedef uint8_t rte_port_id;

    >     

    >     I don't think that it is a good change to do because we will have to

    >     create additional typedef at least for PRIu8. This will look ugly.

    > 

    > No, see my following diff

    > 

    >     Also, if DPDK someday will create typedef with different name

    >     we will have typedef of the typedef?

    > 

    > I don’t follow this comment; see the diff below.

    > 

    > The reasons for the typedef here are:

    > 1) Easier to maintain if the size of type changes in future

    
    In case of future type change we will have to change all the "PRIu8"
    format strings to something else. This is the half of all the changes.
    So, maintainability is in question.

The alternative is to change both PRIu8 and the other code as well…
This reasoning would imply that your proposed change is more maintainable
because all code using the datatype would need to be re-written ?

The main purpose here is to differentiate b/w ovs and external port_id
namespaces. 
The reason why this patch exists is due to confusion in this regard –
using the wrong datatype of int for port_ids derived from the RTE library.

Your patch is trying again to manually replicate the RTE port_id datatype in OVS code and
doing this without documenting that the port_id namespace is RTE and not one of 
OVS native port ids.
This is confusing to me and not maintainable.

    
    > 2) Serves as documentation that the origin of the type is the rte library and

    > and originates externally to ovs

    
    IMHO, using 'rte_' prefix for something defined not inside DPDK is a
    bad style and may be misleading. We should remember that this type
    defined locally in OVS.

This patch is trying to replicate the datatype coming from the RTE library inside
OVS code.
As mentioned earlier, I would prefer the port_id being defined in RTE code and then used in OVS
but I don’t see that datatype defined in the RTE library.

similarly to “struct rte_eth_link”, for example.

Documenting the port_id is derived from rte is needed here, whether it be
rte_port_id
or
ovs_rte_port_id


    
    > 

    > Here is a few examples of typedefs in the OVS code base:

    > 

    > dpif-netdev.c:5587:    typedef uint32_t map_type;

    > stp.h:41:typedef uint64_t stp_identifier;

    > timeval.c:45:typedef unsigned int clockid_t;

    > 

    > 

    > dball@ubuntu:~/ovs$ git diff

    > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c

    > index 609b8da..6667274 100644

    > --- a/lib/netdev-dpdk.c

    > +++ b/lib/netdev-dpdk.c

    > @@ -143,6 +143,9 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))

    >  #define VHOST_ENQ_RETRY_NUM 8

    >  #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)

    >  

    > +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS

    > +typedef uint8_t rte_port_id;

    > +

    >  static const struct rte_eth_conf port_conf = {

    >      .rxmode = {

    >          .mq_mode = ETH_MQ_RX_RSS,

    > @@ -309,7 +312,7 @@ struct dpdk_ring {

    >      struct rte_ring *cring_tx;

    >      struct rte_ring *cring_rx;

    >      unsigned int user_port_id; /* User given port no, parsed from port name */

    > -    int eth_port_id; /* ethernet device port id */

    > +    rte_port_id eth_port_id; /* ethernet device port id */

    >      struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);

    >  };

    >  

    > @@ -325,7 +328,7 @@ enum dpdk_hw_ol_features {

    >  

    >  struct netdev_dpdk {

    >      struct netdev up;

    > -    int port_id;

    > +    rte_port_id port_id;

    >      int max_packet_len;

    >      enum dpdk_dev_type type;

    >  

    > @@ -399,7 +402,7 @@ struct netdev_dpdk {

    >  

    >  struct netdev_rxq_dpdk {

    >      struct netdev_rxq up;

    > -    int port_id;

    > +    rte_port_id port_id;

    >  };

    >  

    >  static int netdev_dpdk_class_init(void);

    > @@ -600,12 +603,12 @@ check_link_status(struct netdev_dpdk *dev)

    >          dev->link_reset_cnt++;

    >          dev->link = link;

    >          if (dev->link.link_status) {

    > -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",

    > +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",

    >                          dev->port_id, (unsigned) dev->link.link_speed,

    >                          (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?

    >                           ("full-duplex") : ("half-duplex"));

    >          } else {

    > -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);

    > +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);

    >          }

    >      }

    >  }

    > @@ -723,7 +726,7 @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)

    >      if (rx_csum_ol_flag &&

    >          (info.rx_offload_capa & rx_chksm_offload_capa) !=

    >           rx_chksm_offload_capa) {

    > -        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %d",

    > +        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %"PRIu8,

    >                     dev->port_id);

    >          dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;

    >          return;

    > @@ -735,7 +738,8 @@ static void

    >  dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)

    >  {

    >      if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {

    > -        VLOG_WARN("Failed to enable flow control on device %d", dev->port_id);

    > +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,

    > +                  dev->port_id);

    >      }

    >  }

    >  

    > @@ -773,7 +777,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >  

    >      memset(&eth_addr, 0x0, sizeof(eth_addr));

    >      rte_eth_macaddr_get(dev->port_id, &eth_addr);

    > -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,

    > +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,

    >                      dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));

    >  

    >      memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);

    > @@ -785,7 +789,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >      /* Get the Flow control configuration for DPDK-ETH */

    >      diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);

    >      if (diag) {

    > -        VLOG_DBG("cannot get flow control parameters on port=%d, err=%d",

    > +        VLOG_DBG("cannot get flow control parameters on port=%"PRIu8", err=%d",

    >                   dev->port_id, diag);

    >      }

    >  

    > @@ -830,7 +834,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)

    >  }

    >  

    >  static int

    > -common_construct(struct netdev *netdev, unsigned int port_no,

    > +common_construct(struct netdev *netdev, rte_port_id port_no,

    >                   enum dpdk_dev_type type, int socket_id)

    >      OVS_REQUIRES(dpdk_mutex)

    >  {

    > @@ -914,7 +918,8 @@ vhost_common_construct(struct netdev *netdev)

    >          return ENOMEM;

    >      }

    >  

    > -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);

    > +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    > +                            DPDK_DEV_VHOST, socket_id);

    >  }

    >  

    >  static int

    > @@ -974,7 +979,8 @@ netdev_dpdk_construct(struct netdev *netdev)

    >      int err;

    >  

    >      ovs_mutex_lock(&dpdk_mutex);

    > -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);

    > +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    > +                           DPDK_DEV_ETH, SOCKET0);

    >      ovs_mutex_unlock(&dpdk_mutex);

    >      return err;

    >  }

    > @@ -1097,7 +1103,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)

    >  }

    >  

    >  static struct netdev_dpdk *

    > -netdev_dpdk_lookup_by_port_id(int port_id)

    > +netdev_dpdk_lookup_by_port_id(rte_port_id port_id)

    >      OVS_REQUIRES(dpdk_mutex)

    >  {

    >      struct netdev_dpdk *dev;

    > @@ -1111,7 +1117,7 @@ netdev_dpdk_lookup_by_port_id(int port_id)

    >      return NULL;

    >  }

    >  

    > -static int

    > +static rte_port_id

    >  netdev_dpdk_process_devargs(const char *devargs, char **errp)

    >  {

    >      uint8_t new_port_id = UINT8_MAX;

    > @@ -1126,7 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)

    >          } else {

    >              /* Attach unsuccessful */

    >              VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);

    > -            return -1;

    > +            new_port_id = DPDK_ETH_PORT_ID_INVALID;

    >          }

    >      }

    >  

    > @@ -1206,7 +1212,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,

    >           * is valid */

    >          if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)

    >                 && rte_eth_dev_is_valid_port(dev->port_id))) {

    > -            int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);

    > +            rte_port_id new_port_id = netdev_dpdk_process_devargs(new_devargs,

    > +                                                                  errp);

    >              if (!rte_eth_dev_is_valid_port(new_port_id)) {

    >                  err = EINVAL;

    >              } else if (new_port_id == dev->port_id) {

    > @@ -2028,7 +2035,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >      int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;

    >  

    >      if (rte_eth_stats_get(dev->port_id, &rte_stats)) {

    > -        VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);

    > +        VLOG_ERR("Can't get ETH statistics for port: %"PRIu8, dev->port_id);

    >          ovs_mutex_unlock(&dev->mutex);

    >          return EPROTO;

    >      }

    > @@ -2036,7 +2043,8 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >      /* Get length of statistics */

    >      rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);

    >      if (rte_xstats_len < 0) {

    > -        VLOG_WARN("Cannot get XSTATS values for port: %i", dev->port_id);

    > +        VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id);

    > +

    >          goto out;

    >      }

    >      /* Reserve memory for xstats names and values */

    > @@ -2059,7 +2067,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >          netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,

    >                                     rte_xstats_len);

    >      } else {

    > -        VLOG_WARN("Cannot get XSTATS values for port: %i.", dev->port_id);

    > +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);

    >      }

    >  

    >  out:

    > @@ -2785,7 +2793,7 @@ netdev_dpdk_vhost_class_init(void)

    >  

    >  static int

    >  dpdk_ring_create(const char dev_name[], unsigned int port_no,

    > -                 unsigned int *eth_port_id)

    > +                 rte_port_id *eth_port_id)

    >  {

    >      struct dpdk_ring *ring_pair;

    >      char *ring_name;

    > @@ -2837,7 +2845,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no,

    >  }

    >  

    >  static int

    > -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)

    > +dpdk_ring_open(const char dev_name[], rte_port_id *eth_port_id)

    >      OVS_REQUIRES(dpdk_mutex)

    >  {

    >      struct dpdk_ring *ring_pair;

    > @@ -2886,7 +2894,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,

    >  static int

    >  netdev_dpdk_ring_construct(struct netdev *netdev)

    >  {

    > -    unsigned int port_no = 0;

    > +    rte_port_id port_no = 0;

    >      int err = 0;

    >  

    >      ovs_mutex_lock(&dpdk_mutex);

    > (END)

    > 

    > 

    > 

    > 

    > 

    >     

    >     > 

    >     >          struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);

    >     >      };

    >     >      

    >     >     @@ -325,7 +327,7 @@ enum dpdk_hw_ol_features {

    >     >      

    >     >      struct netdev_dpdk {

    >     >          struct netdev up;

    >     >     -    int port_id;

    >     >     +    uint8_t port_id;

    >     >          int max_packet_len;

    >     >          enum dpdk_dev_type type;

    >     >      

    >     >     @@ -402,7 +404,7 @@ struct netdev_dpdk {

    >     >      

    >     >      struct netdev_rxq_dpdk {

    >     >          struct netdev_rxq up;

    >     >     -    int port_id;

    >     >     +    uint8_t port_id;

    >     >      };

    >     >      

    >     >      static int netdev_dpdk_class_init(void);

    >     >     @@ -602,12 +604,12 @@ check_link_status(struct netdev_dpdk *dev)

    >     >              dev->link_reset_cnt++;

    >     >              dev->link = link;

    >     >              if (dev->link.link_status) {

    >     >     -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",

    >     >     +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",

    >     >                              dev->port_id, (unsigned) dev->link.link_speed,

    >     >                              (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?

    >     >                               ("full-duplex") : ("half-duplex"));

    >     >              } else {

    >     >     -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);

    >     >     +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);

    >     >              }

    >     >          }

    >     >      }

    >     >     @@ -725,8 +727,8 @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)

    >     >          if (rx_csum_ol_flag &&

    >     >              (info.rx_offload_capa & rx_chksm_offload_capa) !=

    >     >               rx_chksm_offload_capa) {

    >     >     -        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %d",

    >     >     -                   dev->port_id);

    >     >     +        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %"PRIu8,

    >     >     +                       dev->port_id);

    >     >              dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;

    >     >              return;

    >     >          }

    >     >     @@ -737,7 +739,8 @@ static void

    >     >      dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)

    >     >      {

    >     >          if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {

    >     >     -        VLOG_WARN("Failed to enable flow control on device %d", dev->port_id);

    >     >     +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,

    >     >     +                  dev->port_id);

    >     >          }

    >     >      }

    >     >      

    >     >     @@ -775,7 +778,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >     >      

    >     >          memset(&eth_addr, 0x0, sizeof(eth_addr));

    >     >          rte_eth_macaddr_get(dev->port_id, &eth_addr);

    >     >     -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,

    >     >     +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,

    >     >                          dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));

    >     >      

    >     >          memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);

    >     >     @@ -787,7 +790,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >     >          /* Get the Flow control configuration for DPDK-ETH */

    >     >          diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);

    >     >          if (diag) {

    >     >     -        VLOG_DBG("cannot get flow control parameters on port=%d, err=%d",

    >     >     +        VLOG_DBG("cannot get flow control parameters on port=%"PRIu8", err=%d",

    >     >                       dev->port_id, diag);

    >     >          }

    >     >      

    >     >     @@ -832,7 +835,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)

    >     >      }

    >     >      

    >     >      static int

    >     >     -common_construct(struct netdev *netdev, unsigned int port_no,

    >     >     +common_construct(struct netdev *netdev, uint8_t port_no,

    >     >                       enum dpdk_dev_type type, int socket_id)

    >     >          OVS_REQUIRES(dpdk_mutex)

    >     >      {

    >     >     @@ -917,7 +920,8 @@ vhost_common_construct(struct netdev *netdev)

    >     >              return ENOMEM;

    >     >          }

    >     >      

    >     >     -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);

    >     >     +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    >     >     +                            DPDK_DEV_VHOST, socket_id);

    >     >      }

    >     >      

    >     >      static int

    >     >     @@ -977,7 +981,8 @@ netdev_dpdk_construct(struct netdev *netdev)

    >     >          int err;

    >     >      

    >     >          ovs_mutex_lock(&dpdk_mutex);

    >     >     -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);

    >     >     +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    >     >     +                           DPDK_DEV_ETH, SOCKET0);

    >     >          ovs_mutex_unlock(&dpdk_mutex);

    >     >          return err;

    >     >      }

    >     >     @@ -1111,7 +1116,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)

    >     >      }

    >     >      

    >     >      static struct netdev_dpdk *

    >     >     -netdev_dpdk_lookup_by_port_id(int port_id)

    >     >     +netdev_dpdk_lookup_by_port_id(uint8_t port_id)

    >     >          OVS_REQUIRES(dpdk_mutex)

    >     >      {

    >     >          struct netdev_dpdk *dev;

    >     >     @@ -1125,10 +1130,11 @@ netdev_dpdk_lookup_by_port_id(int port_id)

    >     >          return NULL;

    >     >      }

    >     >      

    >     >     -static int

    >     >     -netdev_dpdk_process_devargs(const char *devargs, char **errp)

    >     >     +static uint8_t

    >     >     +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,

    >     >     +                            const char *devargs, char **errp)

    >     >      {

    >     >     -    uint8_t new_port_id = UINT8_MAX;

    >     >     +    uint8_t new_port_id = DPDK_ETH_PORT_ID_INVALID;

    >     >          char *ind, *name = xstrdup(devargs);

    >     >      

    >     >          /* Get the name from the comma separated list of arguments. */

    >     >     @@ -1148,7 +1154,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)

    >     >              } else {

    >     >                  /* Attach unsuccessful */

    >     >                  VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);

    >     >     -            return -1;

    >     >     +            new_port_id = DPDK_ETH_PORT_ID_INVALID;

    >     >              }

    >     >          }

    >     >      

    >     >     @@ -1229,7 +1235,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,

    >     >               * is valid */

    >     >              if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)

    >     >                     && rte_eth_dev_is_valid_port(dev->port_id))) {

    >     >     -            int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);

    >     >     +            uint8_t new_port_id = netdev_dpdk_process_devargs(dev, new_devargs,

    >     >     +                                                              errp);

    >     >                  if (!rte_eth_dev_is_valid_port(new_port_id)) {

    >     >                      err = EINVAL;

    >     >                  } else if (new_port_id == dev->port_id) {

    >     >     @@ -1237,6 +1244,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,

    >     >                      err = 0;

    >     >                  } else {

    >     >                      struct netdev_dpdk *dup_dev;

    >     >     +

    >     >                      dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);

    >     >                      if (dup_dev) {

    >     >                          VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "

    >     >     @@ -1246,6 +1254,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,

    >     >                          err = EADDRINUSE;

    >     >                      } else {

    >     >                          int sid = rte_eth_dev_socket_id(new_port_id);

    >     >     +

    >     >                          dev->requested_socket_id = sid < 0 ? SOCKET0 : sid;

    >     >                          dev->devargs = xstrdup(new_devargs);

    >     >                          dev->port_id = new_port_id;

    >     >     @@ -2051,7 +2060,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >     >          int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;

    >     >      

    >     >          if (rte_eth_stats_get(dev->port_id, &rte_stats)) {

    >     >     -        VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);

    >     >     +        VLOG_ERR("Can't get ETH statistics for port: %"PRIu8, dev->port_id);

    >     >              ovs_mutex_unlock(&dev->mutex);

    >     >              return EPROTO;

    >     >          }

    >     >     @@ -2059,7 +2068,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >     >          /* Get length of statistics */

    >     >          rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);

    >     >          if (rte_xstats_len < 0) {

    >     >     -        VLOG_WARN("Cannot get XSTATS values for port: %i", dev->port_id);

    >     >     +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);

    >     >              goto out;

    >     >          }

    >     >          /* Reserve memory for xstats names and values */

    >     >     @@ -2071,7 +2080,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >     >                                                        rte_xstats_names,

    >     >                                                        rte_xstats_len);

    >     >          if (rte_xstats_new_len != rte_xstats_len) {

    >     >     -        VLOG_WARN("Cannot get XSTATS names for port: %i.", dev->port_id);

    >     >     +        VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id);

    >     >              goto out;

    >     >          }

    >     >          /* Retreive xstats values */

    >     >     @@ -2082,7 +2091,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)

    >     >              netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,

    >     >                                         rte_xstats_len);

    >     >          } else {

    >     >     -        VLOG_WARN("Cannot get XSTATS values for port: %i.", dev->port_id);

    >     >     +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);

    >     >          }

    >     >      

    >     >      out:

    >     >     @@ -2759,7 +2768,7 @@ netdev_dpdk_vhost_class_init(void)

    >     >      

    >     >      static int

    >     >      dpdk_ring_create(const char dev_name[], unsigned int port_no,

    >     >     -                 unsigned int *eth_port_id)

    >     >     +                 uint8_t *eth_port_id)

    >     >      {

    >     >          struct dpdk_ring *ring_pair;

    >     >          char *ring_name;

    >     >     @@ -2811,7 +2820,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no,

    >     >      }

    >     >      

    >     >      static int

    >     >     -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)

    >     >     +dpdk_ring_open(const char dev_name[], uint8_t *eth_port_id)

    >     >          OVS_REQUIRES(dpdk_mutex)

    >     >      {

    >     >          struct dpdk_ring *ring_pair;

    >     >     @@ -2860,7 +2869,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,

    >     >      static int

    >     >      netdev_dpdk_ring_construct(struct netdev *netdev)

    >     >      {

    >     >     -    unsigned int port_no = 0;

    >     >     +    uint8_t port_no = 0;

    >     >          int err = 0;

    >     >      

    >     >          ovs_mutex_lock(&dpdk_mutex);

    >     >     -- 

    >     >     2.7.4

    >     >     

    >     >     _______________________________________________

    >     >     dev mailing list

    >     >     dev@openvswitch.org

    >     >     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=CsrWZo9SNec-_DnojNdU2sRwAPpUXsyy5cTNk5hkhaQ&s=6OANH43fV6QTakoajvWrqyZoezwlGPQF4pLjlhA50ss&e= 

    >     >     

    >     > 

    >     

    >

On 17.05.2017 18:32, Darrell Ball wrote:
> 
> 
> On 5/17/17, 7:59 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:
> 
>     I guess, we need some more opinions about this.
>     
>     My comments inline.
>     
>     Best regards, Ilya Maximets.
>     
>     On 13.05.2017 07:00, Darrell Ball wrote:
>     > 
>     > 
>     > On 5/12/17, 8:04 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:
>     > 
>     >     On 05.04.2017 22:34, Darrell Ball wrote:
>     >     > 
>     >     > 
>     >     > On 4/3/17, 8:04 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ilya Maximets" <ovs-dev-bounces@openvswitch.org on behalf of i.maximets@samsung.com> wrote:
>     >     > 
>     >     >     Currently, signed integer is used for 'port_id' variable and
>     >     >     '-1' as identifier of bad or uninitialized 'port_id'.
>     >     >     
>     >     >     This inconsistent with dpdk library and, also, in few cases,
>     >     >     leads to passing '-1' to dpdk functions where uint8_t expected.
>     >     >     
>     >     >     Such behaviour doesn't produce any issues, but it's better to
>     >     >     use same type as in dpdk library for consistency.
>     >     >     
>     >     >     Also, magic number '-1' replaced with DPDK_ETH_PORT_ID_INVALID
>     >     >     macro.
>     >     >     
>     >     >     Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>     >     >     ---
>     >     >      lib/netdev-dpdk.c | 61 +++++++++++++++++++++++++++++++------------------------
>     >     >      1 file changed, 35 insertions(+), 26 deletions(-)
>     >     >     
>     >     >     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>     >     >     index 658a454..216ced8 100644
>     >     >     --- a/lib/netdev-dpdk.c
>     >     >     +++ b/lib/netdev-dpdk.c
>     >     >     @@ -140,6 +140,8 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>     >     >      #define OVS_VHOST_QUEUE_DISABLED    (-2) /* Queue was disabled by guest and not
>     >     >                                                * yet mapped to another queue. */
>     >     >      
>     >     >     +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS
>     >     >     +
>     >     >      #define VHOST_ENQ_RETRY_NUM 8
>     >     >      #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>     >     >      
>     >     >     @@ -309,7 +311,7 @@ struct dpdk_ring {
>     >     >          struct rte_ring *cring_tx;
>     >     >          struct rte_ring *cring_rx;
>     >     >          unsigned int user_port_id; /* User given port no, parsed from port name */
>     >     >     -    int eth_port_id; /* ethernet device port id */
>     >     >     +    uint8_t eth_port_id; /* ethernet device port id */
>     >     > 
>     >     > The rte does not have a typedef for port_id.
>     >     > One optional change is for OVS to have a typedef for this external type.
>     >     > /* The dpdk rte uses uint8_t for port_id. */
>     >     > typedef uint8_t rte_port_id;
>     >     
>     >     I don't think that it is a good change to do because we will have to
>     >     create additional typedef at least for PRIu8. This will look ugly.
>     > 
>     > No, see my following diff
>     > 
>     >     Also, if DPDK someday will create typedef with different name
>     >     we will have typedef of the typedef?
>     > 
>     > I don’t follow this comment; see the diff below.
>     > 
>     > The reasons for the typedef here are:
>     > 1) Easier to maintain if the size of type changes in future
>     
>     In case of future type change we will have to change all the "PRIu8"
>     format strings to something else. This is the half of all the changes.
>     So, maintainability is in question.
> 
> The alternative is to change both PRIu8 and the other code as well…
> This reasoning would imply that your proposed change is more maintainable
> because all code using the datatype would need to be re-written ?

Maybe you're right, but both approaches are more or less poorly maintainable.
 
> The main purpose here is to differentiate b/w ovs and external port_id
> namespaces. 
> The reason why this patch exists is due to confusion in this regard –
> using the wrong datatype of int for port_ids derived from the RTE library.
> 
> Your patch is trying again to manually replicate the RTE port_id datatype in OVS code and
> doing this without documenting that the port_id namespace is RTE and not one of 
> OVS native port ids.
> This is confusing to me and not maintainable.
> 
>     
>     > 2) Serves as documentation that the origin of the type is the rte library and
>     > and originates externally to ovs
>     
>     IMHO, using 'rte_' prefix for something defined not inside DPDK is a
>     bad style and may be misleading. We should remember that this type
>     defined locally in OVS.
> 
> This patch is trying to replicate the datatype coming from the RTE library inside
> OVS code.
> As mentioned earlier, I would prefer the port_id being defined in RTE code and then used in OVS
> but I don’t see that datatype defined in the RTE library.
> 
> similarly to “struct rte_eth_link”, for example.
> 
> Documenting the port_id is derived from rte is needed here, whether it be
> rte_port_id
> or
> ovs_rte_port_id

OK.
I don't like 'rte_port_id' because it's not defined inside DPDK.
'ovs_rte_port_id' looks too complex.

What do you think about 'dpdk_port_t'?
It will show that this port is from DPDK and will not create a false
sensation that type defined inside the DPDK library.

Additionally it will look similar to other port id types in OVS:

include/openvswitch/types.h:

	typedef uint32_t OVS_BITWISE ofp_port_t;
	typedef uint32_t OVS_BITWISE odp_port_t;
	typedef uint32_t OVS_BITWISE ofp11_port_t;


>     > 
>     > Here is a few examples of typedefs in the OVS code base:
>     > 
>     > dpif-netdev.c:5587:    typedef uint32_t map_type;
>     > stp.h:41:typedef uint64_t stp_identifier;
>     > timeval.c:45:typedef unsigned int clockid_t;
>     > 
>     > 
>     > dball@ubuntu:~/ovs$ git diff
>     > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>     > index 609b8da..6667274 100644
>     > --- a/lib/netdev-dpdk.c
>     > +++ b/lib/netdev-dpdk.c
>     > @@ -143,6 +143,9 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>     >  #define VHOST_ENQ_RETRY_NUM 8
>     >  #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>     >  
>     > +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS
>     > +typedef uint8_t rte_port_id;
>     > +
>     >  static const struct rte_eth_conf port_conf = {
>     >      .rxmode = {
>     >          .mq_mode = ETH_MQ_RX_RSS,
>     > @@ -309,7 +312,7 @@ struct dpdk_ring {
>     >      struct rte_ring *cring_tx;
>     >      struct rte_ring *cring_rx;
>     >      unsigned int user_port_id; /* User given port no, parsed from port name */
>     > -    int eth_port_id; /* ethernet device port id */
>     > +    rte_port_id eth_port_id; /* ethernet device port id */
>     >      struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>     >  };
>     >  
>     > @@ -325,7 +328,7 @@ enum dpdk_hw_ol_features {
>     >  
>     >  struct netdev_dpdk {
>     >      struct netdev up;
>     > -    int port_id;
>     > +    rte_port_id port_id;
>     >      int max_packet_len;
>     >      enum dpdk_dev_type type;
>     >  
>     > @@ -399,7 +402,7 @@ struct netdev_dpdk {
>     >  
>     >  struct netdev_rxq_dpdk {
>     >      struct netdev_rxq up;
>     > -    int port_id;
>     > +    rte_port_id port_id;
>     >  };
>     >  
>     >  static int netdev_dpdk_class_init(void);
>     > @@ -600,12 +603,12 @@ check_link_status(struct netdev_dpdk *dev)
>     >          dev->link_reset_cnt++;
>     >          dev->link = link;
>     >          if (dev->link.link_status) {
>     > -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
>     > +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
>     >                          dev->port_id, (unsigned) dev->link.link_speed,
>     >                          (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>     >                           ("full-duplex") : ("half-duplex"));
>     >          } else {
>     > -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
>     > +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
>     >          }
>     >      }
>     >  }
>     > @@ -723,7 +726,7 @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)
>     >      if (rx_csum_ol_flag &&
>     >          (info.rx_offload_capa & rx_chksm_offload_capa) !=
>     >           rx_chksm_offload_capa) {
>     > -        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %d",
>     > +        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %"PRIu8,
>     >                     dev->port_id);
>     >          dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>     >          return;
>     > @@ -735,7 +738,8 @@ static void
>     >  dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)
>     >  {
>     >      if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
>     > -        VLOG_WARN("Failed to enable flow control on device %d", dev->port_id);
>     > +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,
>     > +                  dev->port_id);
>     >      }
>     >  }
>     >  
>     > @@ -773,7 +777,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>     >  
>     >      memset(&eth_addr, 0x0, sizeof(eth_addr));
>     >      rte_eth_macaddr_get(dev->port_id, &eth_addr);
>     > -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
>     > +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
>     >                      dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>     >  
>     >      memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
>     > @@ -785,7 +789,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>     >      /* Get the Flow control configuration for DPDK-ETH */
>     >      diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
>     >      if (diag) {
>     > -        VLOG_DBG("cannot get flow control parameters on port=%d, err=%d",
>     > +        VLOG_DBG("cannot get flow control parameters on port=%"PRIu8", err=%d",
>     >                   dev->port_id, diag);
>     >      }
>     >  
>     > @@ -830,7 +834,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
>     >  }
>     >  
>     >  static int
>     > -common_construct(struct netdev *netdev, unsigned int port_no,
>     > +common_construct(struct netdev *netdev, rte_port_id port_no,
>     >                   enum dpdk_dev_type type, int socket_id)
>     >      OVS_REQUIRES(dpdk_mutex)
>     >  {
>     > @@ -914,7 +918,8 @@ vhost_common_construct(struct netdev *netdev)
>     >          return ENOMEM;
>     >      }
>     >  
>     > -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);
>     > +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>     > +                            DPDK_DEV_VHOST, socket_id);
>     >  }
>     >  
>     >  static int
>     > @@ -974,7 +979,8 @@ netdev_dpdk_construct(struct netdev *netdev)
>     >      int err;
>     >  
>     >      ovs_mutex_lock(&dpdk_mutex);
>     > -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
>     > +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>     > +                           DPDK_DEV_ETH, SOCKET0);
>     >      ovs_mutex_unlock(&dpdk_mutex);
>     >      return err;
>     >  }
>     > @@ -1097,7 +1103,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)
>     >  }
>     >  
>     >  static struct netdev_dpdk *
>     > -netdev_dpdk_lookup_by_port_id(int port_id)
>     > +netdev_dpdk_lookup_by_port_id(rte_port_id port_id)
>     >      OVS_REQUIRES(dpdk_mutex)
>     >  {
>     >      struct netdev_dpdk *dev;
>     > @@ -1111,7 +1117,7 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>     >      return NULL;
>     >  }
>     >  
>     > -static int
>     > +static rte_port_id
>     >  netdev_dpdk_process_devargs(const char *devargs, char **errp)
>     >  {
>     >      uint8_t new_port_id = UINT8_MAX;
>     > @@ -1126,7 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
>     >          } else {
>     >              /* Attach unsuccessful */
>     >              VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
>     > -            return -1;
>     > +            new_port_id = DPDK_ETH_PORT_ID_INVALID;
>     >          }
>     >      }
>     >  
>     > @@ -1206,7 +1212,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>     >           * is valid */
>     >          if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
>     >                 && rte_eth_dev_is_valid_port(dev->port_id))) {
>     > -            int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);
>     > +            rte_port_id new_port_id = netdev_dpdk_process_devargs(new_devargs,
>     > +                                                                  errp);
>     >              if (!rte_eth_dev_is_valid_port(new_port_id)) {
>     >                  err = EINVAL;
>     >              } else if (new_port_id == dev->port_id) {
>     > @@ -2028,7 +2035,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >      int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
>     >  
>     >      if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
>     > -        VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);
>     > +        VLOG_ERR("Can't get ETH statistics for port: %"PRIu8, dev->port_id);
>     >          ovs_mutex_unlock(&dev->mutex);
>     >          return EPROTO;
>     >      }
>     > @@ -2036,7 +2043,8 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >      /* Get length of statistics */
>     >      rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
>     >      if (rte_xstats_len < 0) {
>     > -        VLOG_WARN("Cannot get XSTATS values for port: %i", dev->port_id);
>     > +        VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id);
>     > +
>     >          goto out;
>     >      }
>     >      /* Reserve memory for xstats names and values */
>     > @@ -2059,7 +2067,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >          netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
>     >                                     rte_xstats_len);
>     >      } else {
>     > -        VLOG_WARN("Cannot get XSTATS values for port: %i.", dev->port_id);
>     > +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);
>     >      }
>     >  
>     >  out:
>     > @@ -2785,7 +2793,7 @@ netdev_dpdk_vhost_class_init(void)
>     >  
>     >  static int
>     >  dpdk_ring_create(const char dev_name[], unsigned int port_no,
>     > -                 unsigned int *eth_port_id)
>     > +                 rte_port_id *eth_port_id)
>     >  {
>     >      struct dpdk_ring *ring_pair;
>     >      char *ring_name;
>     > @@ -2837,7 +2845,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no,
>     >  }
>     >  
>     >  static int
>     > -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)
>     > +dpdk_ring_open(const char dev_name[], rte_port_id *eth_port_id)
>     >      OVS_REQUIRES(dpdk_mutex)
>     >  {
>     >      struct dpdk_ring *ring_pair;
>     > @@ -2886,7 +2894,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,
>     >  static int
>     >  netdev_dpdk_ring_construct(struct netdev *netdev)
>     >  {
>     > -    unsigned int port_no = 0;
>     > +    rte_port_id port_no = 0;
>     >      int err = 0;
>     >  
>     >      ovs_mutex_lock(&dpdk_mutex);
>     > (END)
>     > 
>     > 
>     > 
>     > 
>     > 
>     >     
>     >     > 
>     >     >          struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>     >     >      };
>     >     >      
>     >     >     @@ -325,7 +327,7 @@ enum dpdk_hw_ol_features {
>     >     >      
>     >     >      struct netdev_dpdk {
>     >     >          struct netdev up;
>     >     >     -    int port_id;
>     >     >     +    uint8_t port_id;
>     >     >          int max_packet_len;
>     >     >          enum dpdk_dev_type type;
>     >     >      
>     >     >     @@ -402,7 +404,7 @@ struct netdev_dpdk {
>     >     >      
>     >     >      struct netdev_rxq_dpdk {
>     >     >          struct netdev_rxq up;
>     >     >     -    int port_id;
>     >     >     +    uint8_t port_id;
>     >     >      };
>     >     >      
>     >     >      static int netdev_dpdk_class_init(void);
>     >     >     @@ -602,12 +604,12 @@ check_link_status(struct netdev_dpdk *dev)
>     >     >              dev->link_reset_cnt++;
>     >     >              dev->link = link;
>     >     >              if (dev->link.link_status) {
>     >     >     -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
>     >     >     +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
>     >     >                              dev->port_id, (unsigned) dev->link.link_speed,
>     >     >                              (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>     >     >                               ("full-duplex") : ("half-duplex"));
>     >     >              } else {
>     >     >     -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
>     >     >     +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
>     >     >              }
>     >     >          }
>     >     >      }
>     >     >     @@ -725,8 +727,8 @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)
>     >     >          if (rx_csum_ol_flag &&
>     >     >              (info.rx_offload_capa & rx_chksm_offload_capa) !=
>     >     >               rx_chksm_offload_capa) {
>     >     >     -        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %d",
>     >     >     -                   dev->port_id);
>     >     >     +        VLOG_WARN_ONCE("Rx checksum offload is not supported on device %"PRIu8,
>     >     >     +                       dev->port_id);
>     >     >              dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>     >     >              return;
>     >     >          }
>     >     >     @@ -737,7 +739,8 @@ static void
>     >     >      dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)
>     >     >      {
>     >     >          if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
>     >     >     -        VLOG_WARN("Failed to enable flow control on device %d", dev->port_id);
>     >     >     +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,
>     >     >     +                  dev->port_id);
>     >     >          }
>     >     >      }
>     >     >      
>     >     >     @@ -775,7 +778,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>     >     >      
>     >     >          memset(&eth_addr, 0x0, sizeof(eth_addr));
>     >     >          rte_eth_macaddr_get(dev->port_id, &eth_addr);
>     >     >     -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
>     >     >     +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
>     >     >                          dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>     >     >      
>     >     >          memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
>     >     >     @@ -787,7 +790,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>     >     >          /* Get the Flow control configuration for DPDK-ETH */
>     >     >          diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
>     >     >          if (diag) {
>     >     >     -        VLOG_DBG("cannot get flow control parameters on port=%d, err=%d",
>     >     >     +        VLOG_DBG("cannot get flow control parameters on port=%"PRIu8", err=%d",
>     >     >                       dev->port_id, diag);
>     >     >          }
>     >     >      
>     >     >     @@ -832,7 +835,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
>     >     >      }
>     >     >      
>     >     >      static int
>     >     >     -common_construct(struct netdev *netdev, unsigned int port_no,
>     >     >     +common_construct(struct netdev *netdev, uint8_t port_no,
>     >     >                       enum dpdk_dev_type type, int socket_id)
>     >     >          OVS_REQUIRES(dpdk_mutex)
>     >     >      {
>     >     >     @@ -917,7 +920,8 @@ vhost_common_construct(struct netdev *netdev)
>     >     >              return ENOMEM;
>     >     >          }
>     >     >      
>     >     >     -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);
>     >     >     +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>     >     >     +                            DPDK_DEV_VHOST, socket_id);
>     >     >      }
>     >     >      
>     >     >      static int
>     >     >     @@ -977,7 +981,8 @@ netdev_dpdk_construct(struct netdev *netdev)
>     >     >          int err;
>     >     >      
>     >     >          ovs_mutex_lock(&dpdk_mutex);
>     >     >     -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
>     >     >     +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>     >     >     +                           DPDK_DEV_ETH, SOCKET0);
>     >     >          ovs_mutex_unlock(&dpdk_mutex);
>     >     >          return err;
>     >     >      }
>     >     >     @@ -1111,7 +1116,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)
>     >     >      }
>     >     >      
>     >     >      static struct netdev_dpdk *
>     >     >     -netdev_dpdk_lookup_by_port_id(int port_id)
>     >     >     +netdev_dpdk_lookup_by_port_id(uint8_t port_id)
>     >     >          OVS_REQUIRES(dpdk_mutex)
>     >     >      {
>     >     >          struct netdev_dpdk *dev;
>     >     >     @@ -1125,10 +1130,11 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>     >     >          return NULL;
>     >     >      }
>     >     >      
>     >     >     -static int
>     >     >     -netdev_dpdk_process_devargs(const char *devargs, char **errp)
>     >     >     +static uint8_t
>     >     >     +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
>     >     >     +                            const char *devargs, char **errp)
>     >     >      {
>     >     >     -    uint8_t new_port_id = UINT8_MAX;
>     >     >     +    uint8_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
>     >     >          char *ind, *name = xstrdup(devargs);
>     >     >      
>     >     >          /* Get the name from the comma separated list of arguments. */
>     >     >     @@ -1148,7 +1154,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
>     >     >              } else {
>     >     >                  /* Attach unsuccessful */
>     >     >                  VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
>     >     >     -            return -1;
>     >     >     +            new_port_id = DPDK_ETH_PORT_ID_INVALID;
>     >     >              }
>     >     >          }
>     >     >      
>     >     >     @@ -1229,7 +1235,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>     >     >               * is valid */
>     >     >              if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
>     >     >                     && rte_eth_dev_is_valid_port(dev->port_id))) {
>     >     >     -            int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);
>     >     >     +            uint8_t new_port_id = netdev_dpdk_process_devargs(dev, new_devargs,
>     >     >     +                                                              errp);
>     >     >                  if (!rte_eth_dev_is_valid_port(new_port_id)) {
>     >     >                      err = EINVAL;
>     >     >                  } else if (new_port_id == dev->port_id) {
>     >     >     @@ -1237,6 +1244,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>     >     >                      err = 0;
>     >     >                  } else {
>     >     >                      struct netdev_dpdk *dup_dev;
>     >     >     +
>     >     >                      dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
>     >     >                      if (dup_dev) {
>     >     >                          VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "
>     >     >     @@ -1246,6 +1254,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>     >     >                          err = EADDRINUSE;
>     >     >                      } else {
>     >     >                          int sid = rte_eth_dev_socket_id(new_port_id);
>     >     >     +
>     >     >                          dev->requested_socket_id = sid < 0 ? SOCKET0 : sid;
>     >     >                          dev->devargs = xstrdup(new_devargs);
>     >     >                          dev->port_id = new_port_id;
>     >     >     @@ -2051,7 +2060,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >     >          int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
>     >     >      
>     >     >          if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
>     >     >     -        VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);
>     >     >     +        VLOG_ERR("Can't get ETH statistics for port: %"PRIu8, dev->port_id);
>     >     >              ovs_mutex_unlock(&dev->mutex);
>     >     >              return EPROTO;
>     >     >          }
>     >     >     @@ -2059,7 +2068,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >     >          /* Get length of statistics */
>     >     >          rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
>     >     >          if (rte_xstats_len < 0) {
>     >     >     -        VLOG_WARN("Cannot get XSTATS values for port: %i", dev->port_id);
>     >     >     +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);
>     >     >              goto out;
>     >     >          }
>     >     >          /* Reserve memory for xstats names and values */
>     >     >     @@ -2071,7 +2080,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >     >                                                        rte_xstats_names,
>     >     >                                                        rte_xstats_len);
>     >     >          if (rte_xstats_new_len != rte_xstats_len) {
>     >     >     -        VLOG_WARN("Cannot get XSTATS names for port: %i.", dev->port_id);
>     >     >     +        VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id);
>     >     >              goto out;
>     >     >          }
>     >     >          /* Retreive xstats values */
>     >     >     @@ -2082,7 +2091,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
>     >     >              netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
>     >     >                                         rte_xstats_len);
>     >     >          } else {
>     >     >     -        VLOG_WARN("Cannot get XSTATS values for port: %i.", dev->port_id);
>     >     >     +        VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);
>     >     >          }
>     >     >      
>     >     >      out:
>     >     >     @@ -2759,7 +2768,7 @@ netdev_dpdk_vhost_class_init(void)
>     >     >      
>     >     >      static int
>     >     >      dpdk_ring_create(const char dev_name[], unsigned int port_no,
>     >     >     -                 unsigned int *eth_port_id)
>     >     >     +                 uint8_t *eth_port_id)
>     >     >      {
>     >     >          struct dpdk_ring *ring_pair;
>     >     >          char *ring_name;
>     >     >     @@ -2811,7 +2820,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no,
>     >     >      }
>     >     >      
>     >     >      static int
>     >     >     -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)
>     >     >     +dpdk_ring_open(const char dev_name[], uint8_t *eth_port_id)
>     >     >          OVS_REQUIRES(dpdk_mutex)
>     >     >      {
>     >     >          struct dpdk_ring *ring_pair;
>     >     >     @@ -2860,7 +2869,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,
>     >     >      static int
>     >     >      netdev_dpdk_ring_construct(struct netdev *netdev)
>     >     >      {
>     >     >     -    unsigned int port_no = 0;
>     >     >     +    uint8_t port_no = 0;
>     >     >          int err = 0;
>     >     >      
>     >     >          ovs_mutex_lock(&dpdk_mutex);
>     >     >     -- 
>     >     >     2.7.4
>     >     >     
>     >     >     _______________________________________________
>     >     >     dev mailing list
>     >     >     dev@openvswitch.org
>     >     >     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=CsrWZo9SNec-_DnojNdU2sRwAPpUXsyy5cTNk5hkhaQ&s=6OANH43fV6QTakoajvWrqyZoezwlGPQF4pLjlhA50ss&e= 
>     >     >     
>     >     > 
>     >     
>     > 
>     
>

Hi Ilya,

Ilya Maximets <i.maximets@samsung.com> writes:

> On 17.05.2017 18:32, Darrell Ball wrote:
>> 
>> 
>> On 5/17/17, 7:59 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:
>> 
>>     I guess, we need some more opinions about this.
>>     
>>     My comments inline.
>>     
>>     Best regards, Ilya Maximets.
>>     
>>     On 13.05.2017 07:00, Darrell Ball wrote:
>>     > 
>>     > 
>>     > On 5/12/17, 8:04 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:
>>     > 
>>     >     On 05.04.2017 22:34, Darrell Ball wrote:
>>     >     > 
>>     >     > 
>> > > On 4/3/17, 8:04 AM, "ovs-dev-bounces@openvswitch.org on behalf
> of Ilya Maximets" <ovs-dev-bounces@openvswitch.org on behalf of
> i.maximets@samsung.com> wrote:
>>     >     > 
>>     >     >     Currently, signed integer is used for 'port_id' variable and
>>     >     >     '-1' as identifier of bad or uninitialized 'port_id'.
>>     >     >     
>>     >     >     This inconsistent with dpdk library and, also, in few cases,
>> > > leads to passing '-1' to dpdk functions where uint8_t expected.
>>     >     >     
>>     >     >     Such behaviour doesn't produce any issues, but it's better to
>>     >     >     use same type as in dpdk library for consistency.
>>     >     >     
>>     >     >     Also, magic number '-1' replaced with DPDK_ETH_PORT_ID_INVALID
>>     >     >     macro.
>>     >     >     
>>     >     >     Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>     >     >     ---
>> > > lib/netdev-dpdk.c | 61
> +++++++++++++++++++++++++++++++------------------------
>>     >     >      1 file changed, 35 insertions(+), 26 deletions(-)
>>     >     >     
>>     >     >     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>     >     >     index 658a454..216ced8 100644
>>     >     >     --- a/lib/netdev-dpdk.c
>>     >     >     +++ b/lib/netdev-dpdk.c
>> > > @@ -140,6 +140,8 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /
> ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>> > > #define OVS_VHOST_QUEUE_DISABLED (-2) /* Queue was disabled by
> guest and not
>> > > * yet mapped to another queue. */
>>     >     >      
>>     >     >     +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS
>>     >     >     +
>>     >     >      #define VHOST_ENQ_RETRY_NUM 8
>> > > #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>>     >     >      
>>     >     >     @@ -309,7 +311,7 @@ struct dpdk_ring {
>>     >     >          struct rte_ring *cring_tx;
>>     >     >          struct rte_ring *cring_rx;
>> > > unsigned int user_port_id; /* User given port no, parsed from
> port name */
>>     >     >     -    int eth_port_id; /* ethernet device port id */
>>     >     >     +    uint8_t eth_port_id; /* ethernet device port id */
>>     >     > 
>>     >     > The rte does not have a typedef for port_id.
>> > > One optional change is for OVS to have a typedef for this
> external type.
>>     >     > /* The dpdk rte uses uint8_t for port_id. */
>>     >     > typedef uint8_t rte_port_id;
>>     >     
>>     >     I don't think that it is a good change to do because we will have to
>>     >     create additional typedef at least for PRIu8. This will look ugly.
>>     > 
>>     > No, see my following diff
>>     > 
>>     >     Also, if DPDK someday will create typedef with different name
>>     >     we will have typedef of the typedef?
>>     > 
>>     > I don’t follow this comment; see the diff below.
>>     > 
>>     > The reasons for the typedef here are:
>>     > 1) Easier to maintain if the size of type changes in future
>>     
>>     In case of future type change we will have to change all the "PRIu8"
>>     format strings to something else. This is the half of all the changes.
>>     So, maintainability is in question.
>> 
>> The alternative is to change both PRIu8 and the other code as well…
>> This reasoning would imply that your proposed change is more maintainable
>> because all code using the datatype would need to be re-written ?
>
> Maybe you're right, but both approaches are more or less poorly maintainable.
>  
>> The main purpose here is to differentiate b/w ovs and external port_id
>> namespaces. 
>> The reason why this patch exists is due to confusion in this regard –
>> using the wrong datatype of int for port_ids derived from the RTE library.
>> 
>> Your patch is trying again to manually replicate the RTE port_id
> datatype in OVS code and
>> doing this without documenting that the port_id namespace is RTE and
> not one of
>> OVS native port ids.
>> This is confusing to me and not maintainable.
>> 
>>     
>> > 2) Serves as documentation that the origin of the type is the rte
> library and
>>     > and originates externally to ovs
>>     
>>     IMHO, using 'rte_' prefix for something defined not inside DPDK is a
>>     bad style and may be misleading. We should remember that this type
>>     defined locally in OVS.
>> 
>> This patch is trying to replicate the datatype coming from the RTE
> library inside
>> OVS code.
>> As mentioned earlier, I would prefer the port_id being defined in
> RTE code and then used in OVS
>> but I don’t see that datatype defined in the RTE library.
>> 
>> similarly to “struct rte_eth_link”, for example.
>> 
>> Documenting the port_id is derived from rte is needed here, whether it be
>> rte_port_id
>> or
>> ovs_rte_port_id
>
> OK.
> I don't like 'rte_port_id' because it's not defined inside DPDK.
> 'ovs_rte_port_id' looks too complex.
>
> What do you think about 'dpdk_port_t'?

+1 for this from me.  Since DPDK likes to use rte_X, I was a little
uncomfortable with rte_port_id.  I think this is a good compromise.

No opinion on the print routines.

> It will show that this port is from DPDK and will not create a false
> sensation that type defined inside the DPDK library.
>
> Additionally it will look similar to other port id types in OVS:
>
> include/openvswitch/types.h:
>
> 	typedef uint32_t OVS_BITWISE ofp_port_t;
> 	typedef uint32_t OVS_BITWISE odp_port_t;
> 	typedef uint32_t OVS_BITWISE ofp11_port_t;
>
>
>>     > 
>>     > Here is a few examples of typedefs in the OVS code base:
>>     > 
>>     > dpif-netdev.c:5587:    typedef uint32_t map_type;
>>     > stp.h:41:typedef uint64_t stp_identifier;
>>     > timeval.c:45:typedef unsigned int clockid_t;
>>     > 
>>     > 
>>     > dball@ubuntu:~/ovs$ git diff
>>     > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>     > index 609b8da..6667274 100644
>>     > --- a/lib/netdev-dpdk.c
>>     > +++ b/lib/netdev-dpdk.c
>> > @@ -143,6 +143,9 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /
> ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>>     >  #define VHOST_ENQ_RETRY_NUM 8
>>     >  #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>>     >  
>>     > +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS
>>     > +typedef uint8_t rte_port_id;
>>     > +
>>     >  static const struct rte_eth_conf port_conf = {
>>     >      .rxmode = {
>>     >          .mq_mode = ETH_MQ_RX_RSS,
>>     > @@ -309,7 +312,7 @@ struct dpdk_ring {
>>     >      struct rte_ring *cring_tx;
>>     >      struct rte_ring *cring_rx;
>> > unsigned int user_port_id; /* User given port no, parsed from port
> name */
>>     > -    int eth_port_id; /* ethernet device port id */
>>     > +    rte_port_id eth_port_id; /* ethernet device port id */
>>     >      struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>>     >  };
>>     >  
>>     > @@ -325,7 +328,7 @@ enum dpdk_hw_ol_features {
>>     >  
>>     >  struct netdev_dpdk {
>>     >      struct netdev up;
>>     > -    int port_id;
>>     > +    rte_port_id port_id;
>>     >      int max_packet_len;
>>     >      enum dpdk_dev_type type;
>>     >  
>>     > @@ -399,7 +402,7 @@ struct netdev_dpdk {
>>     >  
>>     >  struct netdev_rxq_dpdk {
>>     >      struct netdev_rxq up;
>>     > -    int port_id;
>>     > +    rte_port_id port_id;
>>     >  };
>>     >  
>>     >  static int netdev_dpdk_class_init(void);
>>     > @@ -600,12 +603,12 @@ check_link_status(struct netdev_dpdk *dev)
>>     >          dev->link_reset_cnt++;
>>     >          dev->link = link;
>>     >          if (dev->link.link_status) {
>>     > -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
>> > + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
>>     >                          dev->port_id, (unsigned) dev->link.link_speed,
>> > (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>     >                           ("full-duplex") : ("half-duplex"));
>>     >          } else {
>>     > -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
>>     > +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
>>     >          }
>>     >      }
>>     >  }
>> > @@ -723,7 +726,7 @@ dpdk_eth_checksum_offload_configure(struct
> netdev_dpdk *dev)
>>     >      if (rx_csum_ol_flag &&
>>     >          (info.rx_offload_capa & rx_chksm_offload_capa) !=
>>     >           rx_chksm_offload_capa) {
>> > - VLOG_WARN_ONCE("Rx checksum offload is not supported on device
> %d",
>> > + VLOG_WARN_ONCE("Rx checksum offload is not supported on device
> %"PRIu8,
>>     >                     dev->port_id);
>>     >          dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>>     >          return;
>>     > @@ -735,7 +738,8 @@ static void
>> > dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev)
> OVS_REQUIRES(dev->mutex)
>>     >  {
>>     >      if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
>> > - VLOG_WARN("Failed to enable flow control on device %d",
> dev->port_id);
>>     > +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,
>>     > +                  dev->port_id);
>>     >      }
>>     >  }
>>     >  
>>     > @@ -773,7 +777,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>     >  
>>     >      memset(&eth_addr, 0x0, sizeof(eth_addr));
>>     >      rte_eth_macaddr_get(dev->port_id, &eth_addr);
>>     > -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
>>     > +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
>> > dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>>     >  
>>     >      memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
>>     > @@ -785,7 +789,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>     >      /* Get the Flow control configuration for DPDK-ETH */
>>     >      diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
>>     >      if (diag) {
>> > - VLOG_DBG("cannot get flow control parameters on port=%d,
> err=%d",
>> > + VLOG_DBG("cannot get flow control parameters on port=%"PRIu8",
> err=%d",
>>     >                   dev->port_id, diag);
>>     >      }
>>     >  
>>     > @@ -830,7 +834,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
>>     >  }
>>     >  
>>     >  static int
>>     > -common_construct(struct netdev *netdev, unsigned int port_no,
>>     > +common_construct(struct netdev *netdev, rte_port_id port_no,
>>     >                   enum dpdk_dev_type type, int socket_id)
>>     >      OVS_REQUIRES(dpdk_mutex)
>>     >  {
>>     > @@ -914,7 +918,8 @@ vhost_common_construct(struct netdev *netdev)
>>     >          return ENOMEM;
>>     >      }
>>     >  
>>     > -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);
>>     > +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>     > +                            DPDK_DEV_VHOST, socket_id);
>>     >  }
>>     >  
>>     >  static int
>>     > @@ -974,7 +979,8 @@ netdev_dpdk_construct(struct netdev *netdev)
>>     >      int err;
>>     >  
>>     >      ovs_mutex_lock(&dpdk_mutex);
>>     > -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
>>     > +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>     > +                           DPDK_DEV_ETH, SOCKET0);
>>     >      ovs_mutex_unlock(&dpdk_mutex);
>>     >      return err;
>>     >  }
>> > @@ -1097,7 +1103,7 @@ netdev_dpdk_get_config(const struct netdev
> *netdev, struct smap *args)
>>     >  }
>>     >  
>>     >  static struct netdev_dpdk *
>>     > -netdev_dpdk_lookup_by_port_id(int port_id)
>>     > +netdev_dpdk_lookup_by_port_id(rte_port_id port_id)
>>     >      OVS_REQUIRES(dpdk_mutex)
>>     >  {
>>     >      struct netdev_dpdk *dev;
>>     > @@ -1111,7 +1117,7 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>>     >      return NULL;
>>     >  }
>>     >  
>>     > -static int
>>     > +static rte_port_id
>>     >  netdev_dpdk_process_devargs(const char *devargs, char **errp)
>>     >  {
>>     >      uint8_t new_port_id = UINT8_MAX;
>> > @@ -1126,7 +1132,7 @@ netdev_dpdk_process_devargs(const char
> *devargs, char **errp)
>>     >          } else {
>>     >              /* Attach unsuccessful */
>> > VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
> devargs);
>>     > -            return -1;
>>     > +            new_port_id = DPDK_ETH_PORT_ID_INVALID;
>>     >          }
>>     >      }
>>     >  
>> > @@ -1206,7 +1212,8 @@ netdev_dpdk_set_config(struct netdev
> *netdev, const struct smap *args,
>>     >           * is valid */
>>     >          if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
>>     >                 && rte_eth_dev_is_valid_port(dev->port_id))) {
>> > - int new_port_id = netdev_dpdk_process_devargs(new_devargs,
> errp);
>> > + rte_port_id new_port_id =
> netdev_dpdk_process_devargs(new_devargs,
>> > + errp);
>>     >              if (!rte_eth_dev_is_valid_port(new_port_id)) {
>>     >                  err = EINVAL;
>>     >              } else if (new_port_id == dev->port_id) {
>> > @@ -2028,7 +2035,7 @@ netdev_dpdk_get_stats(const struct netdev
> *netdev, struct netdev_stats *stats)
>>     >      int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
>>     >  
>>     >      if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
>> > - VLOG_ERR("Can't get ETH statistics for port: %i.",
> dev->port_id);
>> > + VLOG_ERR("Can't get ETH statistics for port: %"PRIu8,
> dev->port_id);
>>     >          ovs_mutex_unlock(&dev->mutex);
>>     >          return EPROTO;
>>     >      }
>> > @@ -2036,7 +2043,8 @@ netdev_dpdk_get_stats(const struct netdev
> *netdev, struct netdev_stats *stats)
>>     >      /* Get length of statistics */
>>     >      rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
>>     >      if (rte_xstats_len < 0) {
>> > - VLOG_WARN("Cannot get XSTATS values for port: %i",
> dev->port_id);
>> > + VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8,
> dev->port_id);
>>     > +
>>     >          goto out;
>>     >      }
>>     >      /* Reserve memory for xstats names and values */
>> > @@ -2059,7 +2067,7 @@ netdev_dpdk_get_stats(const struct netdev
> *netdev, struct netdev_stats *stats)
>>     >          netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
>>     >                                     rte_xstats_len);
>>     >      } else {
>> > - VLOG_WARN("Cannot get XSTATS values for port: %i.",
> dev->port_id);
>> > + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,
> dev->port_id);
>>     >      }
>>     >  
>>     >  out:
>>     > @@ -2785,7 +2793,7 @@ netdev_dpdk_vhost_class_init(void)
>>     >  
>>     >  static int
>>     >  dpdk_ring_create(const char dev_name[], unsigned int port_no,
>>     > -                 unsigned int *eth_port_id)
>>     > +                 rte_port_id *eth_port_id)
>>     >  {
>>     >      struct dpdk_ring *ring_pair;
>>     >      char *ring_name;
>> > @@ -2837,7 +2845,7 @@ dpdk_ring_create(const char dev_name[],
> unsigned int port_no,
>>     >  }
>>     >  
>>     >  static int
>>     > -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)
>>     > +dpdk_ring_open(const char dev_name[], rte_port_id *eth_port_id)
>>     >      OVS_REQUIRES(dpdk_mutex)
>>     >  {
>>     >      struct dpdk_ring *ring_pair;
>> > @@ -2886,7 +2894,7 @@ netdev_dpdk_ring_send(struct netdev *netdev,
> int qid,
>>     >  static int
>>     >  netdev_dpdk_ring_construct(struct netdev *netdev)
>>     >  {
>>     > -    unsigned int port_no = 0;
>>     > +    rte_port_id port_no = 0;
>>     >      int err = 0;
>>     >  
>>     >      ovs_mutex_lock(&dpdk_mutex);
>>     > (END)
>>     > 
>>     > 
>>     > 
>>     > 
>>     > 
>>     >     
>>     >     > 
>>     >     >          struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>>     >     >      };
>>     >     >      
>>     >     >     @@ -325,7 +327,7 @@ enum dpdk_hw_ol_features {
>>     >     >      
>>     >     >      struct netdev_dpdk {
>>     >     >          struct netdev up;
>>     >     >     -    int port_id;
>>     >     >     +    uint8_t port_id;
>>     >     >          int max_packet_len;
>>     >     >          enum dpdk_dev_type type;
>>     >     >      
>>     >     >     @@ -402,7 +404,7 @@ struct netdev_dpdk {
>>     >     >      
>>     >     >      struct netdev_rxq_dpdk {
>>     >     >          struct netdev_rxq up;
>>     >     >     -    int port_id;
>>     >     >     +    uint8_t port_id;
>>     >     >      };
>>     >     >      
>>     >     >      static int netdev_dpdk_class_init(void);
>> > > @@ -602,12 +604,12 @@ check_link_status(struct netdev_dpdk *dev)
>>     >     >              dev->link_reset_cnt++;
>>     >     >              dev->link = link;
>>     >     >              if (dev->link.link_status) {
>> > > - VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
>> > > + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
>> > > dev->port_id, (unsigned) dev->link.link_speed,
>> > > (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>     >     >                               ("full-duplex") : ("half-duplex"));
>>     >     >              } else {
>> > > - VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
>> > > + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
>>     >     >              }
>>     >     >          }
>>     >     >      }
>> > > @@ -725,8 +727,8 @@ dpdk_eth_checksum_offload_configure(struct
> netdev_dpdk *dev)
>>     >     >          if (rx_csum_ol_flag &&
>>     >     >              (info.rx_offload_capa & rx_chksm_offload_capa) !=
>>     >     >               rx_chksm_offload_capa) {
>> > > - VLOG_WARN_ONCE("Rx checksum offload is not supported on device
> %d",
>>     >     >     -                   dev->port_id);
>> > > + VLOG_WARN_ONCE("Rx checksum offload is not supported on device
> %"PRIu8,
>>     >     >     +                       dev->port_id);
>>     >     >              dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>>     >     >              return;
>>     >     >          }
>>     >     >     @@ -737,7 +739,8 @@ static void
>> > > dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev)
> OVS_REQUIRES(dev->mutex)
>>     >     >      {
>> > > if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
>> > > - VLOG_WARN("Failed to enable flow control on device %d",
> dev->port_id);
>> > > + VLOG_WARN("Failed to enable flow control on device %"PRIu8,
>>     >     >     +                  dev->port_id);
>>     >     >          }
>>     >     >      }
>>     >     >      
>>     >     >     @@ -775,7 +778,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>     >     >      
>>     >     >          memset(&eth_addr, 0x0, sizeof(eth_addr));
>>     >     >          rte_eth_macaddr_get(dev->port_id, &eth_addr);
>>     >     >     -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
>>     >     >     +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
>> > > dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>>     >     >      
>> > > memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
>>     >     >     @@ -787,7 +790,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>     >     >          /* Get the Flow control configuration for DPDK-ETH */
>> > > diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
>>     >     >          if (diag) {
>> > > - VLOG_DBG("cannot get flow control parameters on port=%d,
> err=%d",
>> > > + VLOG_DBG("cannot get flow control parameters on port=%"PRIu8",
> err=%d",
>>     >     >                       dev->port_id, diag);
>>     >     >          }
>>     >     >      
>>     >     >     @@ -832,7 +835,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
>>     >     >      }
>>     >     >      
>>     >     >      static int
>>     >     >     -common_construct(struct netdev *netdev, unsigned int port_no,
>>     >     >     +common_construct(struct netdev *netdev, uint8_t port_no,
>>     >     >                       enum dpdk_dev_type type, int socket_id)
>>     >     >          OVS_REQUIRES(dpdk_mutex)
>>     >     >      {
>> > > @@ -917,7 +920,8 @@ vhost_common_construct(struct netdev
> *netdev)
>>     >     >              return ENOMEM;
>>     >     >          }
>>     >     >      
>> > > - return common_construct(netdev, -1, DPDK_DEV_VHOST,
> socket_id);
>>     >     >     +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>     >     >     +                            DPDK_DEV_VHOST, socket_id);
>>     >     >      }
>>     >     >      
>>     >     >      static int
>> > > @@ -977,7 +981,8 @@ netdev_dpdk_construct(struct netdev *netdev)
>>     >     >          int err;
>>     >     >      
>>     >     >          ovs_mutex_lock(&dpdk_mutex);
>> > > - err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
>>     >     >     +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>     >     >     +                           DPDK_DEV_ETH, SOCKET0);
>>     >     >          ovs_mutex_unlock(&dpdk_mutex);
>>     >     >          return err;
>>     >     >      }
>> > > @@ -1111,7 +1116,7 @@ netdev_dpdk_get_config(const struct netdev
> *netdev, struct smap *args)
>>     >     >      }
>>     >     >      
>>     >     >      static struct netdev_dpdk *
>>     >     >     -netdev_dpdk_lookup_by_port_id(int port_id)
>>     >     >     +netdev_dpdk_lookup_by_port_id(uint8_t port_id)
>>     >     >          OVS_REQUIRES(dpdk_mutex)
>>     >     >      {
>>     >     >          struct netdev_dpdk *dev;
>> > > @@ -1125,10 +1130,11 @@ netdev_dpdk_lookup_by_port_id(int
> port_id)
>>     >     >          return NULL;
>>     >     >      }
>>     >     >      
>>     >     >     -static int
>>     >     >     -netdev_dpdk_process_devargs(const char *devargs, char **errp)
>>     >     >     +static uint8_t
>>     >     >     +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
>>     >     >     +                            const char *devargs, char **errp)
>>     >     >      {
>>     >     >     -    uint8_t new_port_id = UINT8_MAX;
>>     >     >     +    uint8_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
>>     >     >          char *ind, *name = xstrdup(devargs);
>>     >     >      
>> > > /* Get the name from the comma separated list of arguments. */
>> > > @@ -1148,7 +1154,7 @@ netdev_dpdk_process_devargs(const char
> *devargs, char **errp)
>>     >     >              } else {
>>     >     >                  /* Attach unsuccessful */
>> > > VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
> devargs);
>>     >     >     -            return -1;
>>     >     >     +            new_port_id = DPDK_ETH_PORT_ID_INVALID;
>>     >     >              }
>>     >     >          }
>>     >     >      
>> > > @@ -1229,7 +1235,8 @@ netdev_dpdk_set_config(struct netdev
> *netdev, const struct smap *args,
>>     >     >               * is valid */
>> > > if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
>>     >     >                     && rte_eth_dev_is_valid_port(dev->port_id))) {
>> > > - int new_port_id = netdev_dpdk_process_devargs(new_devargs,
> errp);
>> > > + uint8_t new_port_id = netdev_dpdk_process_devargs(dev,
> new_devargs,
>> > > + errp);
>>     >     >                  if (!rte_eth_dev_is_valid_port(new_port_id)) {
>>     >     >                      err = EINVAL;
>>     >     >                  } else if (new_port_id == dev->port_id) {
>> > > @@ -1237,6 +1244,7 @@ netdev_dpdk_set_config(struct netdev
> *netdev, const struct smap *args,
>>     >     >                      err = 0;
>>     >     >                  } else {
>>     >     >                      struct netdev_dpdk *dup_dev;
>>     >     >     +
>> > > dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
>>     >     >                      if (dup_dev) {
>> > > VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "
>> > > @@ -1246,6 +1254,7 @@ netdev_dpdk_set_config(struct netdev
> *netdev, const struct smap *args,
>>     >     >                          err = EADDRINUSE;
>>     >     >                      } else {
>> > > int sid = rte_eth_dev_socket_id(new_port_id);
>>     >     >     +
>> > > dev->requested_socket_id = sid < 0 ? SOCKET0 : sid;
>>     >     >                          dev->devargs = xstrdup(new_devargs);
>>     >     >                          dev->port_id = new_port_id;
>> > > @@ -2051,7 +2060,7 @@ netdev_dpdk_get_stats(const struct netdev
> *netdev, struct netdev_stats *stats)
>>     >     >          int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
>>     >     >      
>>     >     >          if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
>> > > - VLOG_ERR("Can't get ETH statistics for port: %i.",
> dev->port_id);
>> > > + VLOG_ERR("Can't get ETH statistics for port: %"PRIu8,
> dev->port_id);
>>     >     >              ovs_mutex_unlock(&dev->mutex);
>>     >     >              return EPROTO;
>>     >     >          }
>> > > @@ -2059,7 +2068,7 @@ netdev_dpdk_get_stats(const struct netdev
> *netdev, struct netdev_stats *stats)
>>     >     >          /* Get length of statistics */
>> > > rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL,
> 0);
>>     >     >          if (rte_xstats_len < 0) {
>> > > - VLOG_WARN("Cannot get XSTATS values for port: %i",
> dev->port_id);
>> > > + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,
> dev->port_id);
>>     >     >              goto out;
>>     >     >          }
>>     >     >          /* Reserve memory for xstats names and values */
>> > > @@ -2071,7 +2080,7 @@ netdev_dpdk_get_stats(const struct netdev
> *netdev, struct netdev_stats *stats)
>> > > rte_xstats_names,
>> > > rte_xstats_len);
>>     >     >          if (rte_xstats_new_len != rte_xstats_len) {
>> > > - VLOG_WARN("Cannot get XSTATS names for port: %i.",
> dev->port_id);
>> > > + VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8,
> dev->port_id);
>>     >     >              goto out;
>>     >     >          }
>>     >     >          /* Retreive xstats values */
>> > > @@ -2082,7 +2091,7 @@ netdev_dpdk_get_stats(const struct netdev
> *netdev, struct netdev_stats *stats)
>> > > netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
>>     >     >                                         rte_xstats_len);
>>     >     >          } else {
>> > > - VLOG_WARN("Cannot get XSTATS values for port: %i.",
> dev->port_id);
>> > > + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,
> dev->port_id);
>>     >     >          }
>>     >     >      
>>     >     >      out:
>>     >     >     @@ -2759,7 +2768,7 @@ netdev_dpdk_vhost_class_init(void)
>>     >     >      
>>     >     >      static int
>>     >     >      dpdk_ring_create(const char dev_name[], unsigned int port_no,
>>     >     >     -                 unsigned int *eth_port_id)
>>     >     >     +                 uint8_t *eth_port_id)
>>     >     >      {
>>     >     >          struct dpdk_ring *ring_pair;
>>     >     >          char *ring_name;
>> > > @@ -2811,7 +2820,7 @@ dpdk_ring_create(const char dev_name[],
> unsigned int port_no,
>>     >     >      }
>>     >     >      
>>     >     >      static int
>> > > -dpdk_ring_open(const char dev_name[], unsigned int
> *eth_port_id)
>>     >     >     +dpdk_ring_open(const char dev_name[], uint8_t *eth_port_id)
>>     >     >          OVS_REQUIRES(dpdk_mutex)
>>     >     >      {
>>     >     >          struct dpdk_ring *ring_pair;
>> > > @@ -2860,7 +2869,7 @@ netdev_dpdk_ring_send(struct netdev
> *netdev, int qid,
>>     >     >      static int
>>     >     >      netdev_dpdk_ring_construct(struct netdev *netdev)
>>     >     >      {
>>     >     >     -    unsigned int port_no = 0;
>>     >     >     +    uint8_t port_no = 0;
>>     >     >          int err = 0;
>>     >     >      
>>     >     >          ovs_mutex_lock(&dpdk_mutex);
>>     >     >     -- 
>>     >     >     2.7.4
>>     >     >     
>>     >     >     _______________________________________________
>>     >     >     dev mailing list
>>     >     >     dev@openvswitch.org
>> > >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=CsrWZo9SNec-_DnojNdU2sRwAPpUXsyy5cTNk5hkhaQ&s=6OANH43fV6QTakoajvWrqyZoezwlGPQF4pLjlhA50ss&e=
>>     >     >     
>>     >     > 
>>     >     
>>     > 
>>     
>>

On 18.05.2017 16:34, Aaron Conole wrote:
> Hi Ilya,
> 
> Ilya Maximets <i.maximets@samsung.com> writes:
> 
>> On 17.05.2017 18:32, Darrell Ball wrote:
>>>
>>>
>>> On 5/17/17, 7:59 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:
>>>
>>>     I guess, we need some more opinions about this.
>>>     
>>>     My comments inline.
>>>     
>>>     Best regards, Ilya Maximets.
>>>     
>>>     On 13.05.2017 07:00, Darrell Ball wrote:
>>>     > 
>>>     > 
>>>     > On 5/12/17, 8:04 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:
>>>     > 
>>>     >     On 05.04.2017 22:34, Darrell Ball wrote:
>>>     >     > 
>>>     >     > 
>>>>> On 4/3/17, 8:04 AM, "ovs-dev-bounces@openvswitch.org on behalf
>> of Ilya Maximets" <ovs-dev-bounces@openvswitch.org on behalf of
>> i.maximets@samsung.com> wrote:
>>>     >     > 
>>>     >     >     Currently, signed integer is used for 'port_id' variable and
>>>     >     >     '-1' as identifier of bad or uninitialized 'port_id'.
>>>     >     >     
>>>     >     >     This inconsistent with dpdk library and, also, in few cases,
>>>>> leads to passing '-1' to dpdk functions where uint8_t expected.
>>>     >     >     
>>>     >     >     Such behaviour doesn't produce any issues, but it's better to
>>>     >     >     use same type as in dpdk library for consistency.
>>>     >     >     
>>>     >     >     Also, magic number '-1' replaced with DPDK_ETH_PORT_ID_INVALID
>>>     >     >     macro.
>>>     >     >     
>>>     >     >     Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>     >     >     ---
>>>>> lib/netdev-dpdk.c | 61
>> +++++++++++++++++++++++++++++++------------------------
>>>     >     >      1 file changed, 35 insertions(+), 26 deletions(-)
>>>     >     >     
>>>     >     >     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>>     >     >     index 658a454..216ced8 100644
>>>     >     >     --- a/lib/netdev-dpdk.c
>>>     >     >     +++ b/lib/netdev-dpdk.c
>>>>> @@ -140,6 +140,8 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /
>> ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>>>>> #define OVS_VHOST_QUEUE_DISABLED (-2) /* Queue was disabled by
>> guest and not
>>>>> * yet mapped to another queue. */
>>>     >     >      
>>>     >     >     +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS
>>>     >     >     +
>>>     >     >      #define VHOST_ENQ_RETRY_NUM 8
>>>>> #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>>>     >     >      
>>>     >     >     @@ -309,7 +311,7 @@ struct dpdk_ring {
>>>     >     >          struct rte_ring *cring_tx;
>>>     >     >          struct rte_ring *cring_rx;
>>>>> unsigned int user_port_id; /* User given port no, parsed from
>> port name */
>>>     >     >     -    int eth_port_id; /* ethernet device port id */
>>>     >     >     +    uint8_t eth_port_id; /* ethernet device port id */
>>>     >     > 
>>>     >     > The rte does not have a typedef for port_id.
>>>>> One optional change is for OVS to have a typedef for this
>> external type.
>>>     >     > /* The dpdk rte uses uint8_t for port_id. */
>>>     >     > typedef uint8_t rte_port_id;
>>>     >     
>>>     >     I don't think that it is a good change to do because we will have to
>>>     >     create additional typedef at least for PRIu8. This will look ugly.
>>>     > 
>>>     > No, see my following diff
>>>     > 
>>>     >     Also, if DPDK someday will create typedef with different name
>>>     >     we will have typedef of the typedef?
>>>     > 
>>>     > I don’t follow this comment; see the diff below.
>>>     > 
>>>     > The reasons for the typedef here are:
>>>     > 1) Easier to maintain if the size of type changes in future
>>>     
>>>     In case of future type change we will have to change all the "PRIu8"
>>>     format strings to something else. This is the half of all the changes.
>>>     So, maintainability is in question.
>>>
>>> The alternative is to change both PRIu8 and the other code as well…
>>> This reasoning would imply that your proposed change is more maintainable
>>> because all code using the datatype would need to be re-written ?
>>
>> Maybe you're right, but both approaches are more or less poorly maintainable.
>>  
>>> The main purpose here is to differentiate b/w ovs and external port_id
>>> namespaces. 
>>> The reason why this patch exists is due to confusion in this regard –
>>> using the wrong datatype of int for port_ids derived from the RTE library.
>>>
>>> Your patch is trying again to manually replicate the RTE port_id
>> datatype in OVS code and
>>> doing this without documenting that the port_id namespace is RTE and
>> not one of
>>> OVS native port ids.
>>> This is confusing to me and not maintainable.
>>>
>>>     
>>>> 2) Serves as documentation that the origin of the type is the rte
>> library and
>>>     > and originates externally to ovs
>>>     
>>>     IMHO, using 'rte_' prefix for something defined not inside DPDK is a
>>>     bad style and may be misleading. We should remember that this type
>>>     defined locally in OVS.
>>>
>>> This patch is trying to replicate the datatype coming from the RTE
>> library inside
>>> OVS code.
>>> As mentioned earlier, I would prefer the port_id being defined in
>> RTE code and then used in OVS
>>> but I don’t see that datatype defined in the RTE library.
>>>
>>> similarly to “struct rte_eth_link”, for example.
>>>
>>> Documenting the port_id is derived from rte is needed here, whether it be
>>> rte_port_id
>>> or
>>> ovs_rte_port_id
>>
>> OK.
>> I don't like 'rte_port_id' because it's not defined inside DPDK.
>> 'ovs_rte_port_id' looks too complex.
>>
>> What do you think about 'dpdk_port_t'?
> 
> +1 for this from me.  Since DPDK likes to use rte_X, I was a little
> uncomfortable with rte_port_id.  I think this is a good compromise.

I've sent v4 with this change.

> No opinion on the print routines.
> 
>> It will show that this port is from DPDK and will not create a false
>> sensation that type defined inside the DPDK library.
>>
>> Additionally it will look similar to other port id types in OVS:
>>
>> include/openvswitch/types.h:
>>
>> 	typedef uint32_t OVS_BITWISE ofp_port_t;
>> 	typedef uint32_t OVS_BITWISE odp_port_t;
>> 	typedef uint32_t OVS_BITWISE ofp11_port_t;
>>
>>
>>>     > 
>>>     > Here is a few examples of typedefs in the OVS code base:
>>>     > 
>>>     > dpif-netdev.c:5587:    typedef uint32_t map_type;
>>>     > stp.h:41:typedef uint64_t stp_identifier;
>>>     > timeval.c:45:typedef unsigned int clockid_t;
>>>     > 
>>>     > 
>>>     > dball@ubuntu:~/ovs$ git diff
>>>     > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>>     > index 609b8da..6667274 100644
>>>     > --- a/lib/netdev-dpdk.c
>>>     > +++ b/lib/netdev-dpdk.c
>>>> @@ -143,6 +143,9 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /
>> ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>>>     >  #define VHOST_ENQ_RETRY_NUM 8
>>>     >  #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>>>     >  
>>>     > +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS
>>>     > +typedef uint8_t rte_port_id;
>>>     > +
>>>     >  static const struct rte_eth_conf port_conf = {
>>>     >      .rxmode = {
>>>     >          .mq_mode = ETH_MQ_RX_RSS,
>>>     > @@ -309,7 +312,7 @@ struct dpdk_ring {
>>>     >      struct rte_ring *cring_tx;
>>>     >      struct rte_ring *cring_rx;
>>>> unsigned int user_port_id; /* User given port no, parsed from port
>> name */
>>>     > -    int eth_port_id; /* ethernet device port id */
>>>     > +    rte_port_id eth_port_id; /* ethernet device port id */
>>>     >      struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>>>     >  };
>>>     >  
>>>     > @@ -325,7 +328,7 @@ enum dpdk_hw_ol_features {
>>>     >  
>>>     >  struct netdev_dpdk {
>>>     >      struct netdev up;
>>>     > -    int port_id;
>>>     > +    rte_port_id port_id;
>>>     >      int max_packet_len;
>>>     >      enum dpdk_dev_type type;
>>>     >  
>>>     > @@ -399,7 +402,7 @@ struct netdev_dpdk {
>>>     >  
>>>     >  struct netdev_rxq_dpdk {
>>>     >      struct netdev_rxq up;
>>>     > -    int port_id;
>>>     > +    rte_port_id port_id;
>>>     >  };
>>>     >  
>>>     >  static int netdev_dpdk_class_init(void);
>>>     > @@ -600,12 +603,12 @@ check_link_status(struct netdev_dpdk *dev)
>>>     >          dev->link_reset_cnt++;
>>>     >          dev->link = link;
>>>     >          if (dev->link.link_status) {
>>>     > -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
>>>> + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
>>>     >                          dev->port_id, (unsigned) dev->link.link_speed,
>>>> (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>>     >                           ("full-duplex") : ("half-duplex"));
>>>     >          } else {
>>>     > -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
>>>     > +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
>>>     >          }
>>>     >      }
>>>     >  }
>>>> @@ -723,7 +726,7 @@ dpdk_eth_checksum_offload_configure(struct
>> netdev_dpdk *dev)
>>>     >      if (rx_csum_ol_flag &&
>>>     >          (info.rx_offload_capa & rx_chksm_offload_capa) !=
>>>     >           rx_chksm_offload_capa) {
>>>> - VLOG_WARN_ONCE("Rx checksum offload is not supported on device
>> %d",
>>>> + VLOG_WARN_ONCE("Rx checksum offload is not supported on device
>> %"PRIu8,
>>>     >                     dev->port_id);
>>>     >          dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>>>     >          return;
>>>     > @@ -735,7 +738,8 @@ static void
>>>> dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev)
>> OVS_REQUIRES(dev->mutex)
>>>     >  {
>>>     >      if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
>>>> - VLOG_WARN("Failed to enable flow control on device %d",
>> dev->port_id);
>>>     > +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,
>>>     > +                  dev->port_id);
>>>     >      }
>>>     >  }
>>>     >  
>>>     > @@ -773,7 +777,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>     >  
>>>     >      memset(&eth_addr, 0x0, sizeof(eth_addr));
>>>     >      rte_eth_macaddr_get(dev->port_id, &eth_addr);
>>>     > -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
>>>     > +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
>>>> dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>>>     >  
>>>     >      memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
>>>     > @@ -785,7 +789,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>     >      /* Get the Flow control configuration for DPDK-ETH */
>>>     >      diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
>>>     >      if (diag) {
>>>> - VLOG_DBG("cannot get flow control parameters on port=%d,
>> err=%d",
>>>> + VLOG_DBG("cannot get flow control parameters on port=%"PRIu8",
>> err=%d",
>>>     >                   dev->port_id, diag);
>>>     >      }
>>>     >  
>>>     > @@ -830,7 +834,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
>>>     >  }
>>>     >  
>>>     >  static int
>>>     > -common_construct(struct netdev *netdev, unsigned int port_no,
>>>     > +common_construct(struct netdev *netdev, rte_port_id port_no,
>>>     >                   enum dpdk_dev_type type, int socket_id)
>>>     >      OVS_REQUIRES(dpdk_mutex)
>>>     >  {
>>>     > @@ -914,7 +918,8 @@ vhost_common_construct(struct netdev *netdev)
>>>     >          return ENOMEM;
>>>     >      }
>>>     >  
>>>     > -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);
>>>     > +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>>     > +                            DPDK_DEV_VHOST, socket_id);
>>>     >  }
>>>     >  
>>>     >  static int
>>>     > @@ -974,7 +979,8 @@ netdev_dpdk_construct(struct netdev *netdev)
>>>     >      int err;
>>>     >  
>>>     >      ovs_mutex_lock(&dpdk_mutex);
>>>     > -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
>>>     > +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>>     > +                           DPDK_DEV_ETH, SOCKET0);
>>>     >      ovs_mutex_unlock(&dpdk_mutex);
>>>     >      return err;
>>>     >  }
>>>> @@ -1097,7 +1103,7 @@ netdev_dpdk_get_config(const struct netdev
>> *netdev, struct smap *args)
>>>     >  }
>>>     >  
>>>     >  static struct netdev_dpdk *
>>>     > -netdev_dpdk_lookup_by_port_id(int port_id)
>>>     > +netdev_dpdk_lookup_by_port_id(rte_port_id port_id)
>>>     >      OVS_REQUIRES(dpdk_mutex)
>>>     >  {
>>>     >      struct netdev_dpdk *dev;
>>>     > @@ -1111,7 +1117,7 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>>>     >      return NULL;
>>>     >  }
>>>     >  
>>>     > -static int
>>>     > +static rte_port_id
>>>     >  netdev_dpdk_process_devargs(const char *devargs, char **errp)
>>>     >  {
>>>     >      uint8_t new_port_id = UINT8_MAX;
>>>> @@ -1126,7 +1132,7 @@ netdev_dpdk_process_devargs(const char
>> *devargs, char **errp)
>>>     >          } else {
>>>     >              /* Attach unsuccessful */
>>>> VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
>> devargs);
>>>     > -            return -1;
>>>     > +            new_port_id = DPDK_ETH_PORT_ID_INVALID;
>>>     >          }
>>>     >      }
>>>     >  
>>>> @@ -1206,7 +1212,8 @@ netdev_dpdk_set_config(struct netdev
>> *netdev, const struct smap *args,
>>>     >           * is valid */
>>>     >          if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
>>>     >                 && rte_eth_dev_is_valid_port(dev->port_id))) {
>>>> - int new_port_id = netdev_dpdk_process_devargs(new_devargs,
>> errp);
>>>> + rte_port_id new_port_id =
>> netdev_dpdk_process_devargs(new_devargs,
>>>> + errp);
>>>     >              if (!rte_eth_dev_is_valid_port(new_port_id)) {
>>>     >                  err = EINVAL;
>>>     >              } else if (new_port_id == dev->port_id) {
>>>> @@ -2028,7 +2035,7 @@ netdev_dpdk_get_stats(const struct netdev
>> *netdev, struct netdev_stats *stats)
>>>     >      int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
>>>     >  
>>>     >      if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
>>>> - VLOG_ERR("Can't get ETH statistics for port: %i.",
>> dev->port_id);
>>>> + VLOG_ERR("Can't get ETH statistics for port: %"PRIu8,
>> dev->port_id);
>>>     >          ovs_mutex_unlock(&dev->mutex);
>>>     >          return EPROTO;
>>>     >      }
>>>> @@ -2036,7 +2043,8 @@ netdev_dpdk_get_stats(const struct netdev
>> *netdev, struct netdev_stats *stats)
>>>     >      /* Get length of statistics */
>>>     >      rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
>>>     >      if (rte_xstats_len < 0) {
>>>> - VLOG_WARN("Cannot get XSTATS values for port: %i",
>> dev->port_id);
>>>> + VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8,
>> dev->port_id);
>>>     > +
>>>     >          goto out;
>>>     >      }
>>>     >      /* Reserve memory for xstats names and values */
>>>> @@ -2059,7 +2067,7 @@ netdev_dpdk_get_stats(const struct netdev
>> *netdev, struct netdev_stats *stats)
>>>     >          netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
>>>     >                                     rte_xstats_len);
>>>     >      } else {
>>>> - VLOG_WARN("Cannot get XSTATS values for port: %i.",
>> dev->port_id);
>>>> + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,
>> dev->port_id);
>>>     >      }
>>>     >  
>>>     >  out:
>>>     > @@ -2785,7 +2793,7 @@ netdev_dpdk_vhost_class_init(void)
>>>     >  
>>>     >  static int
>>>     >  dpdk_ring_create(const char dev_name[], unsigned int port_no,
>>>     > -                 unsigned int *eth_port_id)
>>>     > +                 rte_port_id *eth_port_id)
>>>     >  {
>>>     >      struct dpdk_ring *ring_pair;
>>>     >      char *ring_name;
>>>> @@ -2837,7 +2845,7 @@ dpdk_ring_create(const char dev_name[],
>> unsigned int port_no,
>>>     >  }
>>>     >  
>>>     >  static int
>>>     > -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)
>>>     > +dpdk_ring_open(const char dev_name[], rte_port_id *eth_port_id)
>>>     >      OVS_REQUIRES(dpdk_mutex)
>>>     >  {
>>>     >      struct dpdk_ring *ring_pair;
>>>> @@ -2886,7 +2894,7 @@ netdev_dpdk_ring_send(struct netdev *netdev,
>> int qid,
>>>     >  static int
>>>     >  netdev_dpdk_ring_construct(struct netdev *netdev)
>>>     >  {
>>>     > -    unsigned int port_no = 0;
>>>     > +    rte_port_id port_no = 0;
>>>     >      int err = 0;
>>>     >  
>>>     >      ovs_mutex_lock(&dpdk_mutex);
>>>     > (END)
>>>     > 
>>>     > 
>>>     > 
>>>     > 
>>>     > 
>>>     >     
>>>     >     > 
>>>     >     >          struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>>>     >     >      };
>>>     >     >      
>>>     >     >     @@ -325,7 +327,7 @@ enum dpdk_hw_ol_features {
>>>     >     >      
>>>     >     >      struct netdev_dpdk {
>>>     >     >          struct netdev up;
>>>     >     >     -    int port_id;
>>>     >     >     +    uint8_t port_id;
>>>     >     >          int max_packet_len;
>>>     >     >          enum dpdk_dev_type type;
>>>     >     >      
>>>     >     >     @@ -402,7 +404,7 @@ struct netdev_dpdk {
>>>     >     >      
>>>     >     >      struct netdev_rxq_dpdk {
>>>     >     >          struct netdev_rxq up;
>>>     >     >     -    int port_id;
>>>     >     >     +    uint8_t port_id;
>>>     >     >      };
>>>     >     >      
>>>     >     >      static int netdev_dpdk_class_init(void);
>>>>> @@ -602,12 +604,12 @@ check_link_status(struct netdev_dpdk *dev)
>>>     >     >              dev->link_reset_cnt++;
>>>     >     >              dev->link = link;
>>>     >     >              if (dev->link.link_status) {
>>>>> - VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
>>>>> + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
>>>>> dev->port_id, (unsigned) dev->link.link_speed,
>>>>> (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>>     >     >                               ("full-duplex") : ("half-duplex"));
>>>     >     >              } else {
>>>>> - VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
>>>>> + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
>>>     >     >              }
>>>     >     >          }
>>>     >     >      }
>>>>> @@ -725,8 +727,8 @@ dpdk_eth_checksum_offload_configure(struct
>> netdev_dpdk *dev)
>>>     >     >          if (rx_csum_ol_flag &&
>>>     >     >              (info.rx_offload_capa & rx_chksm_offload_capa) !=
>>>     >     >               rx_chksm_offload_capa) {
>>>>> - VLOG_WARN_ONCE("Rx checksum offload is not supported on device
>> %d",
>>>     >     >     -                   dev->port_id);
>>>>> + VLOG_WARN_ONCE("Rx checksum offload is not supported on device
>> %"PRIu8,
>>>     >     >     +                       dev->port_id);
>>>     >     >              dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>>>     >     >              return;
>>>     >     >          }
>>>     >     >     @@ -737,7 +739,8 @@ static void
>>>>> dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev)
>> OVS_REQUIRES(dev->mutex)
>>>     >     >      {
>>>>> if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
>>>>> - VLOG_WARN("Failed to enable flow control on device %d",
>> dev->port_id);
>>>>> + VLOG_WARN("Failed to enable flow control on device %"PRIu8,
>>>     >     >     +                  dev->port_id);
>>>     >     >          }
>>>     >     >      }
>>>     >     >      
>>>     >     >     @@ -775,7 +778,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>     >     >      
>>>     >     >          memset(&eth_addr, 0x0, sizeof(eth_addr));
>>>     >     >          rte_eth_macaddr_get(dev->port_id, &eth_addr);
>>>     >     >     -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
>>>     >     >     +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
>>>>> dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>>>     >     >      
>>>>> memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
>>>     >     >     @@ -787,7 +790,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>     >     >          /* Get the Flow control configuration for DPDK-ETH */
>>>>> diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
>>>     >     >          if (diag) {
>>>>> - VLOG_DBG("cannot get flow control parameters on port=%d,
>> err=%d",
>>>>> + VLOG_DBG("cannot get flow control parameters on port=%"PRIu8",
>> err=%d",
>>>     >     >                       dev->port_id, diag);
>>>     >     >          }
>>>     >     >      
>>>     >     >     @@ -832,7 +835,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
>>>     >     >      }
>>>     >     >      
>>>     >     >      static int
>>>     >     >     -common_construct(struct netdev *netdev, unsigned int port_no,
>>>     >     >     +common_construct(struct netdev *netdev, uint8_t port_no,
>>>     >     >                       enum dpdk_dev_type type, int socket_id)
>>>     >     >          OVS_REQUIRES(dpdk_mutex)
>>>     >     >      {
>>>>> @@ -917,7 +920,8 @@ vhost_common_construct(struct netdev
>> *netdev)
>>>     >     >              return ENOMEM;
>>>     >     >          }
>>>     >     >      
>>>>> - return common_construct(netdev, -1, DPDK_DEV_VHOST,
>> socket_id);
>>>     >     >     +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>>     >     >     +                            DPDK_DEV_VHOST, socket_id);
>>>     >     >      }
>>>     >     >      
>>>     >     >      static int
>>>>> @@ -977,7 +981,8 @@ netdev_dpdk_construct(struct netdev *netdev)
>>>     >     >          int err;
>>>     >     >      
>>>     >     >          ovs_mutex_lock(&dpdk_mutex);
>>>>> - err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
>>>     >     >     +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>>     >     >     +                           DPDK_DEV_ETH, SOCKET0);
>>>     >     >          ovs_mutex_unlock(&dpdk_mutex);
>>>     >     >          return err;
>>>     >     >      }
>>>>> @@ -1111,7 +1116,7 @@ netdev_dpdk_get_config(const struct netdev
>> *netdev, struct smap *args)
>>>     >     >      }
>>>     >     >      
>>>     >     >      static struct netdev_dpdk *
>>>     >     >     -netdev_dpdk_lookup_by_port_id(int port_id)
>>>     >     >     +netdev_dpdk_lookup_by_port_id(uint8_t port_id)
>>>     >     >          OVS_REQUIRES(dpdk_mutex)
>>>     >     >      {
>>>     >     >          struct netdev_dpdk *dev;
>>>>> @@ -1125,10 +1130,11 @@ netdev_dpdk_lookup_by_port_id(int
>> port_id)
>>>     >     >          return NULL;
>>>     >     >      }
>>>     >     >      
>>>     >     >     -static int
>>>     >     >     -netdev_dpdk_process_devargs(const char *devargs, char **errp)
>>>     >     >     +static uint8_t
>>>     >     >     +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
>>>     >     >     +                            const char *devargs, char **errp)
>>>     >     >      {
>>>     >     >     -    uint8_t new_port_id = UINT8_MAX;
>>>     >     >     +    uint8_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
>>>     >     >          char *ind, *name = xstrdup(devargs);
>>>     >     >      
>>>>> /* Get the name from the comma separated list of arguments. */
>>>>> @@ -1148,7 +1154,7 @@ netdev_dpdk_process_devargs(const char
>> *devargs, char **errp)
>>>     >     >              } else {
>>>     >     >                  /* Attach unsuccessful */
>>>>> VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
>> devargs);
>>>     >     >     -            return -1;
>>>     >     >     +            new_port_id = DPDK_ETH_PORT_ID_INVALID;
>>>     >     >              }
>>>     >     >          }
>>>     >     >      
>>>>> @@ -1229,7 +1235,8 @@ netdev_dpdk_set_config(struct netdev
>> *netdev, const struct smap *args,
>>>     >     >               * is valid */
>>>>> if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
>>>     >     >                     && rte_eth_dev_is_valid_port(dev->port_id))) {
>>>>> - int new_port_id = netdev_dpdk_process_devargs(new_devargs,
>> errp);
>>>>> + uint8_t new_port_id = netdev_dpdk_process_devargs(dev,
>> new_devargs,
>>>>> + errp);
>>>     >     >                  if (!rte_eth_dev_is_valid_port(new_port_id)) {
>>>     >     >                      err = EINVAL;
>>>     >     >                  } else if (new_port_id == dev->port_id) {
>>>>> @@ -1237,6 +1244,7 @@ netdev_dpdk_set_config(struct netdev
>> *netdev, const struct smap *args,
>>>     >     >                      err = 0;
>>>     >     >                  } else {
>>>     >     >                      struct netdev_dpdk *dup_dev;
>>>     >     >     +
>>>>> dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
>>>     >     >                      if (dup_dev) {
>>>>> VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "
>>>>> @@ -1246,6 +1254,7 @@ netdev_dpdk_set_config(struct netdev
>> *netdev, const struct smap *args,
>>>     >     >                          err = EADDRINUSE;
>>>     >     >                      } else {
>>>>> int sid = rte_eth_dev_socket_id(new_port_id);
>>>     >     >     +
>>>>> dev->requested_socket_id = sid < 0 ? SOCKET0 : sid;
>>>     >     >                          dev->devargs = xstrdup(new_devargs);
>>>     >     >                          dev->port_id = new_port_id;
>>>>> @@ -2051,7 +2060,7 @@ netdev_dpdk_get_stats(const struct netdev
>> *netdev, struct netdev_stats *stats)
>>>     >     >          int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
>>>     >     >      
>>>     >     >          if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
>>>>> - VLOG_ERR("Can't get ETH statistics for port: %i.",
>> dev->port_id);
>>>>> + VLOG_ERR("Can't get ETH statistics for port: %"PRIu8,
>> dev->port_id);
>>>     >     >              ovs_mutex_unlock(&dev->mutex);
>>>     >     >              return EPROTO;
>>>     >     >          }
>>>>> @@ -2059,7 +2068,7 @@ netdev_dpdk_get_stats(const struct netdev
>> *netdev, struct netdev_stats *stats)
>>>     >     >          /* Get length of statistics */
>>>>> rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL,
>> 0);
>>>     >     >          if (rte_xstats_len < 0) {
>>>>> - VLOG_WARN("Cannot get XSTATS values for port: %i",
>> dev->port_id);
>>>>> + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,
>> dev->port_id);
>>>     >     >              goto out;
>>>     >     >          }
>>>     >     >          /* Reserve memory for xstats names and values */
>>>>> @@ -2071,7 +2080,7 @@ netdev_dpdk_get_stats(const struct netdev
>> *netdev, struct netdev_stats *stats)
>>>>> rte_xstats_names,
>>>>> rte_xstats_len);
>>>     >     >          if (rte_xstats_new_len != rte_xstats_len) {
>>>>> - VLOG_WARN("Cannot get XSTATS names for port: %i.",
>> dev->port_id);
>>>>> + VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8,
>> dev->port_id);
>>>     >     >              goto out;
>>>     >     >          }
>>>     >     >          /* Retreive xstats values */
>>>>> @@ -2082,7 +2091,7 @@ netdev_dpdk_get_stats(const struct netdev
>> *netdev, struct netdev_stats *stats)
>>>>> netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
>>>     >     >                                         rte_xstats_len);
>>>     >     >          } else {
>>>>> - VLOG_WARN("Cannot get XSTATS values for port: %i.",
>> dev->port_id);
>>>>> + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,
>> dev->port_id);
>>>     >     >          }
>>>     >     >      
>>>     >     >      out:
>>>     >     >     @@ -2759,7 +2768,7 @@ netdev_dpdk_vhost_class_init(void)
>>>     >     >      
>>>     >     >      static int
>>>     >     >      dpdk_ring_create(const char dev_name[], unsigned int port_no,
>>>     >     >     -                 unsigned int *eth_port_id)
>>>     >     >     +                 uint8_t *eth_port_id)
>>>     >     >      {
>>>     >     >          struct dpdk_ring *ring_pair;
>>>     >     >          char *ring_name;
>>>>> @@ -2811,7 +2820,7 @@ dpdk_ring_create(const char dev_name[],
>> unsigned int port_no,
>>>     >     >      }
>>>     >     >      
>>>     >     >      static int
>>>>> -dpdk_ring_open(const char dev_name[], unsigned int
>> *eth_port_id)
>>>     >     >     +dpdk_ring_open(const char dev_name[], uint8_t *eth_port_id)
>>>     >     >          OVS_REQUIRES(dpdk_mutex)
>>>     >     >      {
>>>     >     >          struct dpdk_ring *ring_pair;
>>>>> @@ -2860,7 +2869,7 @@ netdev_dpdk_ring_send(struct netdev
>> *netdev, int qid,
>>>     >     >      static int
>>>     >     >      netdev_dpdk_ring_construct(struct netdev *netdev)
>>>     >     >      {
>>>     >     >     -    unsigned int port_no = 0;
>>>     >     >     +    uint8_t port_no = 0;
>>>     >     >          int err = 0;
>>>     >     >      
>>>     >     >          ovs_mutex_lock(&dpdk_mutex);
>>>     >     >     -- 
>>>     >     >     2.7.4
>>>     >     >     
>>>     >     >     _______________________________________________
>>>     >     >     dev mailing list
>>>     >     >     dev@openvswitch.org
>>>>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=CsrWZo9SNec-_DnojNdU2sRwAPpUXsyy5cTNk5hkhaQ&s=6OANH43fV6QTakoajvWrqyZoezwlGPQF4pLjlhA50ss&e=
>>>     >     >     
>>>     >     > 
>>>     >     
>>>     > 
>>>     
>>>
> 
> 
>

On 5/19/17, 6:39 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:

    On 18.05.2017 16:34, Aaron Conole wrote:
    > Hi Ilya,

    > 

    > Ilya Maximets <i.maximets@samsung.com> writes:

    > 

    >> On 17.05.2017 18:32, Darrell Ball wrote:

    >>>

    >>>

    >>> On 5/17/17, 7:59 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:

    >>>

    >>>     I guess, we need some more opinions about this.

    >>>     

    >>>     My comments inline.

    >>>     

    >>>     Best regards, Ilya Maximets.

    >>>     

    >>>     On 13.05.2017 07:00, Darrell Ball wrote:

    >>>     > 

    >>>     > 

    >>>     > On 5/12/17, 8:04 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:

    >>>     > 

    >>>     >     On 05.04.2017 22:34, Darrell Ball wrote:

    >>>     >     > 

    >>>     >     > 

    >>>>> On 4/3/17, 8:04 AM, "ovs-dev-bounces@openvswitch.org on behalf

    >> of Ilya Maximets" <ovs-dev-bounces@openvswitch.org on behalf of

    >> i.maximets@samsung.com> wrote:

    >>>     >     > 

    >>>     >     >     Currently, signed integer is used for 'port_id' variable and

    >>>     >     >     '-1' as identifier of bad or uninitialized 'port_id'.

    >>>     >     >     

    >>>     >     >     This inconsistent with dpdk library and, also, in few cases,

    >>>>> leads to passing '-1' to dpdk functions where uint8_t expected.

    >>>     >     >     

    >>>     >     >     Such behaviour doesn't produce any issues, but it's better to

    >>>     >     >     use same type as in dpdk library for consistency.

    >>>     >     >     

    >>>     >     >     Also, magic number '-1' replaced with DPDK_ETH_PORT_ID_INVALID

    >>>     >     >     macro.

    >>>     >     >     

    >>>     >     >     Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

    >>>     >     >     ---

    >>>>> lib/netdev-dpdk.c | 61

    >> +++++++++++++++++++++++++++++++------------------------

    >>>     >     >      1 file changed, 35 insertions(+), 26 deletions(-)

    >>>     >     >     

    >>>     >     >     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c

    >>>     >     >     index 658a454..216ced8 100644

    >>>     >     >     --- a/lib/netdev-dpdk.c

    >>>     >     >     +++ b/lib/netdev-dpdk.c

    >>>>> @@ -140,6 +140,8 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /

    >> ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))

    >>>>> #define OVS_VHOST_QUEUE_DISABLED (-2) /* Queue was disabled by

    >> guest and not

    >>>>> * yet mapped to another queue. */

    >>>     >     >      

    >>>     >     >     +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS

    >>>     >     >     +

    >>>     >     >      #define VHOST_ENQ_RETRY_NUM 8

    >>>>> #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)

    >>>     >     >      

    >>>     >     >     @@ -309,7 +311,7 @@ struct dpdk_ring {

    >>>     >     >          struct rte_ring *cring_tx;

    >>>     >     >          struct rte_ring *cring_rx;

    >>>>> unsigned int user_port_id; /* User given port no, parsed from

    >> port name */

    >>>     >     >     -    int eth_port_id; /* ethernet device port id */

    >>>     >     >     +    uint8_t eth_port_id; /* ethernet device port id */

    >>>     >     > 

    >>>     >     > The rte does not have a typedef for port_id.

    >>>>> One optional change is for OVS to have a typedef for this

    >> external type.

    >>>     >     > /* The dpdk rte uses uint8_t for port_id. */

    >>>     >     > typedef uint8_t rte_port_id;

    >>>     >     

    >>>     >     I don't think that it is a good change to do because we will have to

    >>>     >     create additional typedef at least for PRIu8. This will look ugly.

    >>>     > 

    >>>     > No, see my following diff

    >>>     > 

    >>>     >     Also, if DPDK someday will create typedef with different name

    >>>     >     we will have typedef of the typedef?

    >>>     > 

    >>>     > I don’t follow this comment; see the diff below.

    >>>     > 

    >>>     > The reasons for the typedef here are:

    >>>     > 1) Easier to maintain if the size of type changes in future

    >>>     

    >>>     In case of future type change we will have to change all the "PRIu8"

    >>>     format strings to something else. This is the half of all the changes.

    >>>     So, maintainability is in question.

    >>>

    >>> The alternative is to change both PRIu8 and the other code as well…

    >>> This reasoning would imply that your proposed change is more maintainable

    >>> because all code using the datatype would need to be re-written ?

    >>

    >> Maybe you're right, but both approaches are more or less poorly maintainable.

    >>  

    >>> The main purpose here is to differentiate b/w ovs and external port_id

    >>> namespaces. 

    >>> The reason why this patch exists is due to confusion in this regard –

    >>> using the wrong datatype of int for port_ids derived from the RTE library.

    >>>

    >>> Your patch is trying again to manually replicate the RTE port_id

    >> datatype in OVS code and

    >>> doing this without documenting that the port_id namespace is RTE and

    >> not one of

    >>> OVS native port ids.

    >>> This is confusing to me and not maintainable.

    >>>

    >>>     

    >>>> 2) Serves as documentation that the origin of the type is the rte

    >> library and

    >>>     > and originates externally to ovs

    >>>     

    >>>     IMHO, using 'rte_' prefix for something defined not inside DPDK is a

    >>>     bad style and may be misleading. We should remember that this type

    >>>     defined locally in OVS.

    >>>

    >>> This patch is trying to replicate the datatype coming from the RTE

    >> library inside

    >>> OVS code.

    >>> As mentioned earlier, I would prefer the port_id being defined in

    >> RTE code and then used in OVS

    >>> but I don’t see that datatype defined in the RTE library.

    >>>

    >>> similarly to “struct rte_eth_link”, for example.

    >>>

    >>> Documenting the port_id is derived from rte is needed here, whether it be

    >>> rte_port_id

    >>> or

    >>> ovs_rte_port_id

    >>

    >> OK.

    >> I don't like 'rte_port_id' because it's not defined inside DPDK.

    >> 'ovs_rte_port_id' looks too complex.

    >>

    >> What do you think about 'dpdk_port_t'?

    > 

    > +1 for this from me.  Since DPDK likes to use rte_X, I was a little

    > uncomfortable with rte_port_id.  I think this is a good compromise.

    
    I've sent v4 with this change.

The particular name used is not the most important aspect here.

In regards to the rte_ prefix by itself, I agree it is not a good choice.
After all, this is not compat code.
ovs_rte_ has some precedence and implies ovs version of the more specific
library referenced, being “rte”.

However, dpdk_ is already a commonly used prefix here and that is good for 
consistency. 

    
    > No opinion on the print routines.

    > 

    >> It will show that this port is from DPDK and will not create a false

    >> sensation that type defined inside the DPDK library.

    >>

    >> Additionally it will look similar to other port id types in OVS:

    >>

    >> include/openvswitch/types.h:

    >>

    >> 	typedef uint32_t OVS_BITWISE ofp_port_t;

    >> 	typedef uint32_t OVS_BITWISE odp_port_t;

    >> 	typedef uint32_t OVS_BITWISE ofp11_port_t;

    >>

    >>

    >>>     > 

    >>>     > Here is a few examples of typedefs in the OVS code base:

    >>>     > 

    >>>     > dpif-netdev.c:5587:    typedef uint32_t map_type;

    >>>     > stp.h:41:typedef uint64_t stp_identifier;

    >>>     > timeval.c:45:typedef unsigned int clockid_t;

    >>>     > 

    >>>     > 

    >>>     > dball@ubuntu:~/ovs$ git diff

    >>>     > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c

    >>>     > index 609b8da..6667274 100644

    >>>     > --- a/lib/netdev-dpdk.c

    >>>     > +++ b/lib/netdev-dpdk.c

    >>>> @@ -143,6 +143,9 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /

    >> ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))

    >>>     >  #define VHOST_ENQ_RETRY_NUM 8

    >>>     >  #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)

    >>>     >  

    >>>     > +#define DPDK_ETH_PORT_ID_INVALID    RTE_MAX_ETHPORTS

    >>>     > +typedef uint8_t rte_port_id;

    >>>     > +

    >>>     >  static const struct rte_eth_conf port_conf = {

    >>>     >      .rxmode = {

    >>>     >          .mq_mode = ETH_MQ_RX_RSS,

    >>>     > @@ -309,7 +312,7 @@ struct dpdk_ring {

    >>>     >      struct rte_ring *cring_tx;

    >>>     >      struct rte_ring *cring_rx;

    >>>> unsigned int user_port_id; /* User given port no, parsed from port

    >> name */

    >>>     > -    int eth_port_id; /* ethernet device port id */

    >>>     > +    rte_port_id eth_port_id; /* ethernet device port id */

    >>>     >      struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);

    >>>     >  };

    >>>     >  

    >>>     > @@ -325,7 +328,7 @@ enum dpdk_hw_ol_features {

    >>>     >  

    >>>     >  struct netdev_dpdk {

    >>>     >      struct netdev up;

    >>>     > -    int port_id;

    >>>     > +    rte_port_id port_id;

    >>>     >      int max_packet_len;

    >>>     >      enum dpdk_dev_type type;

    >>>     >  

    >>>     > @@ -399,7 +402,7 @@ struct netdev_dpdk {

    >>>     >  

    >>>     >  struct netdev_rxq_dpdk {

    >>>     >      struct netdev_rxq up;

    >>>     > -    int port_id;

    >>>     > +    rte_port_id port_id;

    >>>     >  };

    >>>     >  

    >>>     >  static int netdev_dpdk_class_init(void);

    >>>     > @@ -600,12 +603,12 @@ check_link_status(struct netdev_dpdk *dev)

    >>>     >          dev->link_reset_cnt++;

    >>>     >          dev->link = link;

    >>>     >          if (dev->link.link_status) {

    >>>     > -            VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",

    >>>> + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",

    >>>     >                          dev->port_id, (unsigned) dev->link.link_speed,

    >>>> (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?

    >>>     >                           ("full-duplex") : ("half-duplex"));

    >>>     >          } else {

    >>>     > -            VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);

    >>>     > +            VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);

    >>>     >          }

    >>>     >      }

    >>>     >  }

    >>>> @@ -723,7 +726,7 @@ dpdk_eth_checksum_offload_configure(struct

    >> netdev_dpdk *dev)

    >>>     >      if (rx_csum_ol_flag &&

    >>>     >          (info.rx_offload_capa & rx_chksm_offload_capa) !=

    >>>     >           rx_chksm_offload_capa) {

    >>>> - VLOG_WARN_ONCE("Rx checksum offload is not supported on device

    >> %d",

    >>>> + VLOG_WARN_ONCE("Rx checksum offload is not supported on device

    >> %"PRIu8,

    >>>     >                     dev->port_id);

    >>>     >          dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;

    >>>     >          return;

    >>>     > @@ -735,7 +738,8 @@ static void

    >>>> dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev)

    >> OVS_REQUIRES(dev->mutex)

    >>>     >  {

    >>>     >      if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {

    >>>> - VLOG_WARN("Failed to enable flow control on device %d",

    >> dev->port_id);

    >>>     > +        VLOG_WARN("Failed to enable flow control on device %"PRIu8,

    >>>     > +                  dev->port_id);

    >>>     >      }

    >>>     >  }

    >>>     >  

    >>>     > @@ -773,7 +777,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >>>     >  

    >>>     >      memset(&eth_addr, 0x0, sizeof(eth_addr));

    >>>     >      rte_eth_macaddr_get(dev->port_id, &eth_addr);

    >>>     > -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,

    >>>     > +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,

    >>>> dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));

    >>>     >  

    >>>     >      memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);

    >>>     > @@ -785,7 +789,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >>>     >      /* Get the Flow control configuration for DPDK-ETH */

    >>>     >      diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);

    >>>     >      if (diag) {

    >>>> - VLOG_DBG("cannot get flow control parameters on port=%d,

    >> err=%d",

    >>>> + VLOG_DBG("cannot get flow control parameters on port=%"PRIu8",

    >> err=%d",

    >>>     >                   dev->port_id, diag);

    >>>     >      }

    >>>     >  

    >>>     > @@ -830,7 +834,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)

    >>>     >  }

    >>>     >  

    >>>     >  static int

    >>>     > -common_construct(struct netdev *netdev, unsigned int port_no,

    >>>     > +common_construct(struct netdev *netdev, rte_port_id port_no,

    >>>     >                   enum dpdk_dev_type type, int socket_id)

    >>>     >      OVS_REQUIRES(dpdk_mutex)

    >>>     >  {

    >>>     > @@ -914,7 +918,8 @@ vhost_common_construct(struct netdev *netdev)

    >>>     >          return ENOMEM;

    >>>     >      }

    >>>     >  

    >>>     > -    return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);

    >>>     > +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    >>>     > +                            DPDK_DEV_VHOST, socket_id);

    >>>     >  }

    >>>     >  

    >>>     >  static int

    >>>     > @@ -974,7 +979,8 @@ netdev_dpdk_construct(struct netdev *netdev)

    >>>     >      int err;

    >>>     >  

    >>>     >      ovs_mutex_lock(&dpdk_mutex);

    >>>     > -    err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);

    >>>     > +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    >>>     > +                           DPDK_DEV_ETH, SOCKET0);

    >>>     >      ovs_mutex_unlock(&dpdk_mutex);

    >>>     >      return err;

    >>>     >  }

    >>>> @@ -1097,7 +1103,7 @@ netdev_dpdk_get_config(const struct netdev

    >> *netdev, struct smap *args)

    >>>     >  }

    >>>     >  

    >>>     >  static struct netdev_dpdk *

    >>>     > -netdev_dpdk_lookup_by_port_id(int port_id)

    >>>     > +netdev_dpdk_lookup_by_port_id(rte_port_id port_id)

    >>>     >      OVS_REQUIRES(dpdk_mutex)

    >>>     >  {

    >>>     >      struct netdev_dpdk *dev;

    >>>     > @@ -1111,7 +1117,7 @@ netdev_dpdk_lookup_by_port_id(int port_id)

    >>>     >      return NULL;

    >>>     >  }

    >>>     >  

    >>>     > -static int

    >>>     > +static rte_port_id

    >>>     >  netdev_dpdk_process_devargs(const char *devargs, char **errp)

    >>>     >  {

    >>>     >      uint8_t new_port_id = UINT8_MAX;

    >>>> @@ -1126,7 +1132,7 @@ netdev_dpdk_process_devargs(const char

    >> *devargs, char **errp)

    >>>     >          } else {

    >>>     >              /* Attach unsuccessful */

    >>>> VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",

    >> devargs);

    >>>     > -            return -1;

    >>>     > +            new_port_id = DPDK_ETH_PORT_ID_INVALID;

    >>>     >          }

    >>>     >      }

    >>>     >  

    >>>> @@ -1206,7 +1212,8 @@ netdev_dpdk_set_config(struct netdev

    >> *netdev, const struct smap *args,

    >>>     >           * is valid */

    >>>     >          if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)

    >>>     >                 && rte_eth_dev_is_valid_port(dev->port_id))) {

    >>>> - int new_port_id = netdev_dpdk_process_devargs(new_devargs,

    >> errp);

    >>>> + rte_port_id new_port_id =

    >> netdev_dpdk_process_devargs(new_devargs,

    >>>> + errp);

    >>>     >              if (!rte_eth_dev_is_valid_port(new_port_id)) {

    >>>     >                  err = EINVAL;

    >>>     >              } else if (new_port_id == dev->port_id) {

    >>>> @@ -2028,7 +2035,7 @@ netdev_dpdk_get_stats(const struct netdev

    >> *netdev, struct netdev_stats *stats)

    >>>     >      int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;

    >>>     >  

    >>>     >      if (rte_eth_stats_get(dev->port_id, &rte_stats)) {

    >>>> - VLOG_ERR("Can't get ETH statistics for port: %i.",

    >> dev->port_id);

    >>>> + VLOG_ERR("Can't get ETH statistics for port: %"PRIu8,

    >> dev->port_id);

    >>>     >          ovs_mutex_unlock(&dev->mutex);

    >>>     >          return EPROTO;

    >>>     >      }

    >>>> @@ -2036,7 +2043,8 @@ netdev_dpdk_get_stats(const struct netdev

    >> *netdev, struct netdev_stats *stats)

    >>>     >      /* Get length of statistics */

    >>>     >      rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);

    >>>     >      if (rte_xstats_len < 0) {

    >>>> - VLOG_WARN("Cannot get XSTATS values for port: %i",

    >> dev->port_id);

    >>>> + VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8,

    >> dev->port_id);

    >>>     > +

    >>>     >          goto out;

    >>>     >      }

    >>>     >      /* Reserve memory for xstats names and values */

    >>>> @@ -2059,7 +2067,7 @@ netdev_dpdk_get_stats(const struct netdev

    >> *netdev, struct netdev_stats *stats)

    >>>     >          netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,

    >>>     >                                     rte_xstats_len);

    >>>     >      } else {

    >>>> - VLOG_WARN("Cannot get XSTATS values for port: %i.",

    >> dev->port_id);

    >>>> + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,

    >> dev->port_id);

    >>>     >      }

    >>>     >  

    >>>     >  out:

    >>>     > @@ -2785,7 +2793,7 @@ netdev_dpdk_vhost_class_init(void)

    >>>     >  

    >>>     >  static int

    >>>     >  dpdk_ring_create(const char dev_name[], unsigned int port_no,

    >>>     > -                 unsigned int *eth_port_id)

    >>>     > +                 rte_port_id *eth_port_id)

    >>>     >  {

    >>>     >      struct dpdk_ring *ring_pair;

    >>>     >      char *ring_name;

    >>>> @@ -2837,7 +2845,7 @@ dpdk_ring_create(const char dev_name[],

    >> unsigned int port_no,

    >>>     >  }

    >>>     >  

    >>>     >  static int

    >>>     > -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)

    >>>     > +dpdk_ring_open(const char dev_name[], rte_port_id *eth_port_id)

    >>>     >      OVS_REQUIRES(dpdk_mutex)

    >>>     >  {

    >>>     >      struct dpdk_ring *ring_pair;

    >>>> @@ -2886,7 +2894,7 @@ netdev_dpdk_ring_send(struct netdev *netdev,

    >> int qid,

    >>>     >  static int

    >>>     >  netdev_dpdk_ring_construct(struct netdev *netdev)

    >>>     >  {

    >>>     > -    unsigned int port_no = 0;

    >>>     > +    rte_port_id port_no = 0;

    >>>     >      int err = 0;

    >>>     >  

    >>>     >      ovs_mutex_lock(&dpdk_mutex);

    >>>     > (END)

    >>>     > 

    >>>     > 

    >>>     > 

    >>>     > 

    >>>     > 

    >>>     >     

    >>>     >     > 

    >>>     >     >          struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);

    >>>     >     >      };

    >>>     >     >      

    >>>     >     >     @@ -325,7 +327,7 @@ enum dpdk_hw_ol_features {

    >>>     >     >      

    >>>     >     >      struct netdev_dpdk {

    >>>     >     >          struct netdev up;

    >>>     >     >     -    int port_id;

    >>>     >     >     +    uint8_t port_id;

    >>>     >     >          int max_packet_len;

    >>>     >     >          enum dpdk_dev_type type;

    >>>     >     >      

    >>>     >     >     @@ -402,7 +404,7 @@ struct netdev_dpdk {

    >>>     >     >      

    >>>     >     >      struct netdev_rxq_dpdk {

    >>>     >     >          struct netdev_rxq up;

    >>>     >     >     -    int port_id;

    >>>     >     >     +    uint8_t port_id;

    >>>     >     >      };

    >>>     >     >      

    >>>     >     >      static int netdev_dpdk_class_init(void);

    >>>>> @@ -602,12 +604,12 @@ check_link_status(struct netdev_dpdk *dev)

    >>>     >     >              dev->link_reset_cnt++;

    >>>     >     >              dev->link = link;

    >>>     >     >              if (dev->link.link_status) {

    >>>>> - VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",

    >>>>> + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",

    >>>>> dev->port_id, (unsigned) dev->link.link_speed,

    >>>>> (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?

    >>>     >     >                               ("full-duplex") : ("half-duplex"));

    >>>     >     >              } else {

    >>>>> - VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);

    >>>>> + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);

    >>>     >     >              }

    >>>     >     >          }

    >>>     >     >      }

    >>>>> @@ -725,8 +727,8 @@ dpdk_eth_checksum_offload_configure(struct

    >> netdev_dpdk *dev)

    >>>     >     >          if (rx_csum_ol_flag &&

    >>>     >     >              (info.rx_offload_capa & rx_chksm_offload_capa) !=

    >>>     >     >               rx_chksm_offload_capa) {

    >>>>> - VLOG_WARN_ONCE("Rx checksum offload is not supported on device

    >> %d",

    >>>     >     >     -                   dev->port_id);

    >>>>> + VLOG_WARN_ONCE("Rx checksum offload is not supported on device

    >> %"PRIu8,

    >>>     >     >     +                       dev->port_id);

    >>>     >     >              dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;

    >>>     >     >              return;

    >>>     >     >          }

    >>>     >     >     @@ -737,7 +739,8 @@ static void

    >>>>> dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev)

    >> OVS_REQUIRES(dev->mutex)

    >>>     >     >      {

    >>>>> if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {

    >>>>> - VLOG_WARN("Failed to enable flow control on device %d",

    >> dev->port_id);

    >>>>> + VLOG_WARN("Failed to enable flow control on device %"PRIu8,

    >>>     >     >     +                  dev->port_id);

    >>>     >     >          }

    >>>     >     >      }

    >>>     >     >      

    >>>     >     >     @@ -775,7 +778,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >>>     >     >      

    >>>     >     >          memset(&eth_addr, 0x0, sizeof(eth_addr));

    >>>     >     >          rte_eth_macaddr_get(dev->port_id, &eth_addr);

    >>>     >     >     -    VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,

    >>>     >     >     +    VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,

    >>>>> dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));

    >>>     >     >      

    >>>>> memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);

    >>>     >     >     @@ -787,7 +790,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

    >>>     >     >          /* Get the Flow control configuration for DPDK-ETH */

    >>>>> diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);

    >>>     >     >          if (diag) {

    >>>>> - VLOG_DBG("cannot get flow control parameters on port=%d,

    >> err=%d",

    >>>>> + VLOG_DBG("cannot get flow control parameters on port=%"PRIu8",

    >> err=%d",

    >>>     >     >                       dev->port_id, diag);

    >>>     >     >          }

    >>>     >     >      

    >>>     >     >     @@ -832,7 +835,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)

    >>>     >     >      }

    >>>     >     >      

    >>>     >     >      static int

    >>>     >     >     -common_construct(struct netdev *netdev, unsigned int port_no,

    >>>     >     >     +common_construct(struct netdev *netdev, uint8_t port_no,

    >>>     >     >                       enum dpdk_dev_type type, int socket_id)

    >>>     >     >          OVS_REQUIRES(dpdk_mutex)

    >>>     >     >      {

    >>>>> @@ -917,7 +920,8 @@ vhost_common_construct(struct netdev

    >> *netdev)

    >>>     >     >              return ENOMEM;

    >>>     >     >          }

    >>>     >     >      

    >>>>> - return common_construct(netdev, -1, DPDK_DEV_VHOST,

    >> socket_id);

    >>>     >     >     +    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    >>>     >     >     +                            DPDK_DEV_VHOST, socket_id);

    >>>     >     >      }

    >>>     >     >      

    >>>     >     >      static int

    >>>>> @@ -977,7 +981,8 @@ netdev_dpdk_construct(struct netdev *netdev)

    >>>     >     >          int err;

    >>>     >     >      

    >>>     >     >          ovs_mutex_lock(&dpdk_mutex);

    >>>>> - err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);

    >>>     >     >     +    err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,

    >>>     >     >     +                           DPDK_DEV_ETH, SOCKET0);

    >>>     >     >          ovs_mutex_unlock(&dpdk_mutex);

    >>>     >     >          return err;

    >>>     >     >      }

    >>>>> @@ -1111,7 +1116,7 @@ netdev_dpdk_get_config(const struct netdev

    >> *netdev, struct smap *args)

    >>>     >     >      }

    >>>     >     >      

    >>>     >     >      static struct netdev_dpdk *

    >>>     >     >     -netdev_dpdk_lookup_by_port_id(int port_id)

    >>>     >     >     +netdev_dpdk_lookup_by_port_id(uint8_t port_id)

    >>>     >     >          OVS_REQUIRES(dpdk_mutex)

    >>>     >     >      {

    >>>     >     >          struct netdev_dpdk *dev;

    >>>>> @@ -1125,10 +1130,11 @@ netdev_dpdk_lookup_by_port_id(int

    >> port_id)

    >>>     >     >          return NULL;

    >>>     >     >      }

    >>>     >     >      

    >>>     >     >     -static int

    >>>     >     >     -netdev_dpdk_process_devargs(const char *devargs, char **errp)

    >>>     >     >     +static uint8_t

    >>>     >     >     +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,

    >>>     >     >     +                            const char *devargs, char **errp)

    >>>     >     >      {

    >>>     >     >     -    uint8_t new_port_id = UINT8_MAX;

    >>>     >     >     +    uint8_t new_port_id = DPDK_ETH_PORT_ID_INVALID;

    >>>     >     >          char *ind, *name = xstrdup(devargs);

    >>>     >     >      

    >>>>> /* Get the name from the comma separated list of arguments. */

    >>>>> @@ -1148,7 +1154,7 @@ netdev_dpdk_process_devargs(const char

    >> *devargs, char **errp)

    >>>     >     >              } else {

    >>>     >     >                  /* Attach unsuccessful */

    >>>>> VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",

    >> devargs);

    >>>     >     >     -            return -1;

    >>>     >     >     +            new_port_id = DPDK_ETH_PORT_ID_INVALID;

    >>>     >     >              }

    >>>     >     >          }

    >>>     >     >      

    >>>>> @@ -1229,7 +1235,8 @@ netdev_dpdk_set_config(struct netdev

    >> *netdev, const struct smap *args,

    >>>     >     >               * is valid */

    >>>>> if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)

    >>>     >     >                     && rte_eth_dev_is_valid_port(dev->port_id))) {

    >>>>> - int new_port_id = netdev_dpdk_process_devargs(new_devargs,

    >> errp);

    >>>>> + uint8_t new_port_id = netdev_dpdk_process_devargs(dev,

    >> new_devargs,

    >>>>> + errp);

    >>>     >     >                  if (!rte_eth_dev_is_valid_port(new_port_id)) {

    >>>     >     >                      err = EINVAL;

    >>>     >     >                  } else if (new_port_id == dev->port_id) {

    >>>>> @@ -1237,6 +1244,7 @@ netdev_dpdk_set_config(struct netdev

    >> *netdev, const struct smap *args,

    >>>     >     >                      err = 0;

    >>>     >     >                  } else {

    >>>     >     >                      struct netdev_dpdk *dup_dev;

    >>>     >     >     +

    >>>>> dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);

    >>>     >     >                      if (dup_dev) {

    >>>>> VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "

    >>>>> @@ -1246,6 +1254,7 @@ netdev_dpdk_set_config(struct netdev

    >> *netdev, const struct smap *args,

    >>>     >     >                          err = EADDRINUSE;

    >>>     >     >                      } else {

    >>>>> int sid = rte_eth_dev_socket_id(new_port_id);

    >>>     >     >     +

    >>>>> dev->requested_socket_id = sid < 0 ? SOCKET0 : sid;

    >>>     >     >                          dev->devargs = xstrdup(new_devargs);

    >>>     >     >                          dev->port_id = new_port_id;

    >>>>> @@ -2051,7 +2060,7 @@ netdev_dpdk_get_stats(const struct netdev

    >> *netdev, struct netdev_stats *stats)

    >>>     >     >          int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;

    >>>     >     >      

    >>>     >     >          if (rte_eth_stats_get(dev->port_id, &rte_stats)) {

    >>>>> - VLOG_ERR("Can't get ETH statistics for port: %i.",

    >> dev->port_id);

    >>>>> + VLOG_ERR("Can't get ETH statistics for port: %"PRIu8,

    >> dev->port_id);

    >>>     >     >              ovs_mutex_unlock(&dev->mutex);

    >>>     >     >              return EPROTO;

    >>>     >     >          }

    >>>>> @@ -2059,7 +2068,7 @@ netdev_dpdk_get_stats(const struct netdev

    >> *netdev, struct netdev_stats *stats)

    >>>     >     >          /* Get length of statistics */

    >>>>> rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL,

    >> 0);

    >>>     >     >          if (rte_xstats_len < 0) {

    >>>>> - VLOG_WARN("Cannot get XSTATS values for port: %i",

    >> dev->port_id);

    >>>>> + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,

    >> dev->port_id);

    >>>     >     >              goto out;

    >>>     >     >          }

    >>>     >     >          /* Reserve memory for xstats names and values */

    >>>>> @@ -2071,7 +2080,7 @@ netdev_dpdk_get_stats(const struct netdev

    >> *netdev, struct netdev_stats *stats)

    >>>>> rte_xstats_names,

    >>>>> rte_xstats_len);

    >>>     >     >          if (rte_xstats_new_len != rte_xstats_len) {

    >>>>> - VLOG_WARN("Cannot get XSTATS names for port: %i.",

    >> dev->port_id);

    >>>>> + VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8,

    >> dev->port_id);

    >>>     >     >              goto out;

    >>>     >     >          }

    >>>     >     >          /* Retreive xstats values */

    >>>>> @@ -2082,7 +2091,7 @@ netdev_dpdk_get_stats(const struct netdev

    >> *netdev, struct netdev_stats *stats)

    >>>>> netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,

    >>>     >     >                                         rte_xstats_len);

    >>>     >     >          } else {

    >>>>> - VLOG_WARN("Cannot get XSTATS values for port: %i.",

    >> dev->port_id);

    >>>>> + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,

    >> dev->port_id);

    >>>     >     >          }

    >>>     >     >      

    >>>     >     >      out:

    >>>     >     >     @@ -2759,7 +2768,7 @@ netdev_dpdk_vhost_class_init(void)

    >>>     >     >      

    >>>     >     >      static int

    >>>     >     >      dpdk_ring_create(const char dev_name[], unsigned int port_no,

    >>>     >     >     -                 unsigned int *eth_port_id)

    >>>     >     >     +                 uint8_t *eth_port_id)

    >>>     >     >      {

    >>>     >     >          struct dpdk_ring *ring_pair;

    >>>     >     >          char *ring_name;

    >>>>> @@ -2811,7 +2820,7 @@ dpdk_ring_create(const char dev_name[],

    >> unsigned int port_no,

    >>>     >     >      }

    >>>     >     >      

    >>>     >     >      static int

    >>>>> -dpdk_ring_open(const char dev_name[], unsigned int

    >> *eth_port_id)

    >>>     >     >     +dpdk_ring_open(const char dev_name[], uint8_t *eth_port_id)

    >>>     >     >          OVS_REQUIRES(dpdk_mutex)

    >>>     >     >      {

    >>>     >     >          struct dpdk_ring *ring_pair;

    >>>>> @@ -2860,7 +2869,7 @@ netdev_dpdk_ring_send(struct netdev

    >> *netdev, int qid,

    >>>     >     >      static int

    >>>     >     >      netdev_dpdk_ring_construct(struct netdev *netdev)

    >>>     >     >      {

    >>>     >     >     -    unsigned int port_no = 0;

    >>>     >     >     +    uint8_t port_no = 0;

    >>>     >     >          int err = 0;

    >>>     >     >      

    >>>     >     >          ovs_mutex_lock(&dpdk_mutex);

    >>>     >     >     -- 

    >>>     >     >     2.7.4

    >>>     >     >     

    >>>     >     >     _______________________________________________

    >>>     >     >     dev mailing list

    >>>     >     >     dev@openvswitch.org

    >>>>>

    >> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=CsrWZo9SNec-_DnojNdU2sRwAPpUXsyy5cTNk5hkhaQ&s=6OANH43fV6QTakoajvWrqyZoezwlGPQF4pLjlhA50ss&e=

    >>>     >     >     

    >>>     >     > 

    >>>     >     

    >>>     > 

    >>>     

    >>>

    > 

    > 

    >