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

[Darrell Ball]

On 5/19/17, 6:39 AM, "Ilya Maximets"  wrote:

On 18.05.2017 16:34, Aaron Conole wrote:
> Hi Ilya,
> 
> Ilya Maximets  writes:
> 
>> On 17.05.2017 18:32, Darrell Ball wrote:
>>>
>>>
>>> On 5/17/17, 7:59 AM, "Ilya Maximets"  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"  
wrote:
>>> > 
>>> > On 05.04.2017 22:34, Darrell Ball wrote:
>>> > > 
>>> > > 
> On 4/3/17, 8:04 AM, "ovs-dev-boun...@openvswitch.org on behalf
>> of Ilya Maximets" > i.maxim...@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 
>>> > > ---
> 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_INVALIDRTE_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 

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

[Ilya Maximets]

On 18.05.2017 16:34, Aaron Conole wrote:
> Hi Ilya,
> 
> Ilya Maximets  writes:
> 
>> On 17.05.2017 18:32, Darrell Ball wrote:
>>>
>>>
>>> On 5/17/17, 7:59 AM, "Ilya Maximets"  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"  wrote:
>>> > 
>>> > On 05.04.2017 22:34, Darrell Ball wrote:
>>> > > 
>>> > > 
> On 4/3/17, 8:04 AM, "ovs-dev-boun...@openvswitch.org on behalf
>> of Ilya Maximets" > i.maxim...@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 
>>> > > ---
> 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_INVALIDRTE_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 

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

[Aaron Conole]

Hi Ilya,

Ilya Maximets  writes:

> On 17.05.2017 18:32, Darrell Ball wrote:
>> 
>> 
>> On 5/17/17, 7:59 AM, "Ilya Maximets"  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"  wrote:
>> > 
>> > On 05.04.2017 22:34, Darrell Ball wrote:
>> > > 
>> > > 
>> > > On 4/3/17, 8:04 AM, "ovs-dev-boun...@openvswitch.org on behalf
> of Ilya Maximets"  i.maxim...@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 
>> > > ---
>> > > 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_INVALIDRTE_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 

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

[Ilya Maximets]

On 17.05.2017 18:32, Darrell Ball wrote:
> 
> 
> On 5/17/17, 7:59 AM, "Ilya Maximets"  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"  wrote:
> > 
> > On 05.04.2017 22:34, Darrell Ball wrote:
> > > 
> > > 
> > > On 4/3/17, 8:04 AM, "ovs-dev-boun...@openvswitch.org on behalf of 
> Ilya Maximets"  i.maxim...@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 
> > > ---
> > >  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_INVALIDRTE_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 
> 

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

[Ilya Maximets]

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"  wrote:
> 
> On 05.04.2017 22:34, Darrell Ball wrote:
> > 
> > 
> > On 4/3/17, 8:04 AM, "ovs-dev-boun...@openvswitch.org on behalf of Ilya 
> Maximets"  i.maxim...@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 
> > ---
> >  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_INVALIDRTE_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_INVALIDRTE_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 

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

[Darrell Ball]

On 5/12/17, 8:04 AM, "Ilya Maximets"  wrote:

On 05.04.2017 22:34, Darrell Ball wrote:
> 
> 
> On 4/3/17, 8:04 AM, "ovs-dev-boun...@openvswitch.org on behalf of Ilya 
Maximets"  
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 
> ---
>  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_INVALIDRTE_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
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_INVALIDRTE_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(, "Port %d Link Up - speed %u Mbps - %s",
+VLOG_DBG_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(, "Port %d Link Down", dev->port_id);
+VLOG_DBG_RL(, "Port %"PRIu8" Link 

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

[Ilya Maximets]

On 05.04.2017 22:34, Darrell Ball wrote:
> 
> 
> On 4/3/17, 8:04 AM, "ovs-dev-boun...@openvswitch.org on behalf of Ilya 
> Maximets"  i.maxim...@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 
> ---
>  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_INVALIDRTE_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.
Also, if DPDK someday will create typedef with different name
we will have typedef of the typedef?

> 
>  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(, "Port %d Link Up - speed %u Mbps - %s",
> +VLOG_DBG_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(, "Port %d Link Down", dev->port_id);
> +VLOG_DBG_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, >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(_addr, 0x0, sizeof(eth_addr));
>  rte_eth_macaddr_get(dev->port_id, _addr);
> -VLOG_INFO_RL(, "Port %d: "ETH_ADDR_FMT,
> +VLOG_INFO_RL(, "Port %"PRIu8": "ETH_ADDR_FMT,
>   

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

[Darrell Ball]

On 4/3/17, 8:04 AM, "ovs-dev-boun...@openvswitch.org on behalf of Ilya 
Maximets"  
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 
---
 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_INVALIDRTE_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; 

 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(, "Port %d Link Up - speed %u Mbps - %s",
+VLOG_DBG_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(, "Port %d Link Down", dev->port_id);
+VLOG_DBG_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, >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(_addr, 0x0, sizeof(eth_addr));
 rte_eth_macaddr_get(dev->port_id, _addr);
-VLOG_INFO_RL(, "Port %d: "ETH_ADDR_FMT,
+VLOG_INFO_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, >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", 

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

[Aaron Conole]

Ilya Maximets  writes:

> 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 
> ---

Acked-by: Aaron Conole 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev