diff mbox

[dpdk-dev,v6,4/8] ethdev: Add port representor device flag

Message ID 20180328135433.20203-5-declan.doherty@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Doherty, Declan March 28, 2018, 1:54 p.m. UTC 
Add new device flag to specify that ethdev port is a port representor.
Extend rte_eth_dev_info structure to expose device flags to user which
enable applications to discover if a port is a representor port.

Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
 lib/librte_ether/rte_ethdev.c             | 1 +
 lib/librte_ether/rte_ethdev.h             | 9 ++++++---
 lib/librte_ether/rte_ethdev_representor.h | 3 +++
 3 files changed, 10 insertions(+), 3 deletions(-)

Comments

Shahaf Shuler March 29, 2018, 6:13 a.m. UTC | #1 
Wednesday, March 28, 2018 4:54 PM, Declan Doherty:
> Subject: [dpdk-dev][PATCH v6 4/8] ethdev: Add port representor device flag
> 
> Add new device flag to specify that ethdev port is a port representor.
> Extend rte_eth_dev_info structure to expose device flags to user which
> enable applications to discover if a port is a representor port.
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c             | 1 +
>  lib/librte_ether/rte_ethdev.h             | 9 ++++++---
>  lib/librte_ether/rte_ethdev_representor.h | 3 +++
>  3 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index c719f84a3..163246433 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2399,6 +2399,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct
> rte_eth_dev_info *dev_info)
>  	dev_info->nb_rx_queues = dev->data->nb_rx_queues;
>  	dev_info->nb_tx_queues = dev->data->nb_tx_queues;
>  	dev_info->switch_id = dev->data->switch_id;
> +	dev_info->dev_flags = dev->data->dev_flags;
>  }
> 
>  int
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index dced4fc41..226acc8b1 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -996,6 +996,7 @@ struct rte_eth_dev_info {
>  	const char *driver_name; /**< Device Driver name. */
>  	unsigned int if_index; /**< Index to bound host interface, or 0 if
> none.
>  		Use if_indextoname() to translate into an interface name. */
> +	uint32_t dev_flags; /**< Device flags */
>  	uint32_t min_rx_bufsize; /**< Minimum size of RX buffer. */
>  	uint32_t max_rx_pktlen; /**< Maximum configurable length of RX
> pkt. */
>  	uint16_t max_rx_queues; /**< Maximum number of RX queues. */
> @@ -1229,11 +1230,13 @@ struct rte_eth_dev_owner {  };
> 
>  /** Device supports link state interrupt */
> -#define RTE_ETH_DEV_INTR_LSC     0x0002
> +#define RTE_ETH_DEV_INTR_LSC		0x0002
>  /** Device is a bonded slave */
> -#define RTE_ETH_DEV_BONDED_SLAVE 0x0004
> +#define RTE_ETH_DEV_BONDED_SLAVE	0x0004
>  /** Device supports device removal interrupt */
> -#define RTE_ETH_DEV_INTR_RMV     0x0008
> +#define RTE_ETH_DEV_INTR_RMV		0x0008
> +/** Device is port representor */
> +#define RTE_ETH_DEV_REPRESENTOR		0x0010

Maybe it is a good time to make some order here. 
I understand the decision to use flags instead of bit-field. It is better. 

However there is a mix here of device capabilities like : RTE_ETH_DEV_INTR_LSC   and RTE_ETH_DEV_INTR_RMV   
And device attributes like : RTE_ETH_DEV_BONDED_SLAVE and RTE_ETH_DEV_REPRESENTOR.
I don't think they belong together under the genetic name of dev_flags. 

Moreover, I am not sure the fact device is bonded slave should be exposed to the application. It should be internal to ethdev and its port iterators. 

Finally I think representor port may need more info now (and in the future), for example the associated vf id.
For that, I think it is better it to be exposed as a dedicated struct on device info.

> 
>  /**
>   * @warning
> diff --git a/lib/librte_ether/rte_ethdev_representor.h
> b/lib/librte_ether/rte_ethdev_representor.h
> index cbc1f2855..f3726d0ba 100644
> --- a/lib/librte_ether/rte_ethdev_representor.h
> +++ b/lib/librte_ether/rte_ethdev_representor.h
> @@ -22,6 +22,9 @@ eth_dev_representor_port_init(struct rte_eth_dev
> *ethdev, void *init_params)
>  	/** representor inherits the switch id of it's base device */
>  	ethdev->data->switch_id = base_ethdev->data->switch_id;
> 
> +	/** Set device flags to specify that device is a representor port */
> +	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;

Should be set in the PMD, not in ethdev layer. 

> +
>  	return 0;
>  }
> 
> --
> 2.14.3
Thomas Monjalon March 29, 2018, 7:34 a.m. UTC | #2 
29/03/2018 08:13, Shahaf Shuler:
> And device attributes like : RTE_ETH_DEV_BONDED_SLAVE and RTE_ETH_DEV_REPRESENTOR.
> I don't think they belong together under the genetic name of dev_flags. 
> 
> Moreover, I am not sure the fact device is bonded slave should be exposed to the application. It should be internal to ethdev and its port iterators.

RTE_ETH_DEV_BONDED_SLAVE flag is used to prevent a manual detach of a slave.
I think it is wrong.
The bonding PMD should be able to manage any detach at any time,
because a real hardware plug-out can happen.

So I am in favor of removing RTE_ETH_DEV_BONDED_SLAVE.
Doherty, Declan March 29, 2018, 2:53 p.m. UTC | #3 
On 29/03/2018 7:13 AM, Shahaf Shuler wrote:
> Wednesday, March 28, 2018 4:54 PM, Declan Doherty:
>> Subject: [dpdk-dev][PATCH v6 4/8] ethdev: Add port representor device flag
>>
>> Add new device flag to specify that ethdev port is a port representor.
>> Extend rte_eth_dev_info structure to expose device flags to user which
>> enable applications to discover if a port is a representor port.
>>
>> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
>> ---
>>   lib/librte_ether/rte_ethdev.c             | 1 +
>>   lib/librte_ether/rte_ethdev.h             | 9 ++++++---
>>   lib/librte_ether/rte_ethdev_representor.h | 3 +++
>>   3 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index c719f84a3..163246433 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -2399,6 +2399,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct
>> rte_eth_dev_info *dev_info)
>>   	dev_info->nb_rx_queues = dev->data->nb_rx_queues;
>>   	dev_info->nb_tx_queues = dev->data->nb_tx_queues;
>>   	dev_info->switch_id = dev->data->switch_id;
>> +	dev_info->dev_flags = dev->data->dev_flags;
>>   }
>>
>>   int
>> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
>> index dced4fc41..226acc8b1 100644
>> --- a/lib/librte_ether/rte_ethdev.h
>> +++ b/lib/librte_ether/rte_ethdev.h
>> @@ -996,6 +996,7 @@ struct rte_eth_dev_info {
>>   	const char *driver_name; /**< Device Driver name. */
>>   	unsigned int if_index; /**< Index to bound host interface, or 0 if
>> none.
>>   		Use if_indextoname() to translate into an interface name. */
>> +	uint32_t dev_flags; /**< Device flags */
>>   	uint32_t min_rx_bufsize; /**< Minimum size of RX buffer. */
>>   	uint32_t max_rx_pktlen; /**< Maximum configurable length of RX
>> pkt. */
>>   	uint16_t max_rx_queues; /**< Maximum number of RX queues. */
>> @@ -1229,11 +1230,13 @@ struct rte_eth_dev_owner {  };
>>
>>   /** Device supports link state interrupt */
>> -#define RTE_ETH_DEV_INTR_LSC     0x0002
>> +#define RTE_ETH_DEV_INTR_LSC		0x0002
>>   /** Device is a bonded slave */
>> -#define RTE_ETH_DEV_BONDED_SLAVE 0x0004
>> +#define RTE_ETH_DEV_BONDED_SLAVE	0x0004
>>   /** Device supports device removal interrupt */
>> -#define RTE_ETH_DEV_INTR_RMV     0x0008
>> +#define RTE_ETH_DEV_INTR_RMV		0x0008
>> +/** Device is port representor */
>> +#define RTE_ETH_DEV_REPRESENTOR		0x0010
> 
> Maybe it is a good time to make some order here.
> I understand the decision to use flags instead of bit-field. It is better.
> 
> However there is a mix here of device capabilities like : RTE_ETH_DEV_INTR_LSC   and RTE_ETH_DEV_INTR_RMV
> And device attributes like : RTE_ETH_DEV_BONDED_SLAVE and RTE_ETH_DEV_REPRESENTOR.
> I don't think they belong together under the genetic name of dev_flags.
> 
> Moreover, I am not sure the fact device is bonded slave should be exposed to the application. It should be internal to ethdev and its port iterators.

That's a good point on the bonded slave flag, I'll look at fixing that 
for the next release. I don't think changing it should effect ABI but 
I'll need to have a closer look.

Do you think that we should have a separate device attributes field, 
which the representor flag is contained in.

> 
> Finally I think representor port may need more info now (and in the future), for example the associated vf id.
> For that, I think it is better it to be exposed as a dedicated struct on device info.

I think a switch port id should suffice for that, for SR-IOV devices it 
would map to the vf_id.

> 
>>
>>   /**
>>    * @warning
>> diff --git a/lib/librte_ether/rte_ethdev_representor.h
>> b/lib/librte_ether/rte_ethdev_representor.h
>> index cbc1f2855..f3726d0ba 100644
>> --- a/lib/librte_ether/rte_ethdev_representor.h
>> +++ b/lib/librte_ether/rte_ethdev_representor.h
>> @@ -22,6 +22,9 @@ eth_dev_representor_port_init(struct rte_eth_dev
>> *ethdev, void *init_params)
>>   	/** representor inherits the switch id of it's base device */
>>   	ethdev->data->switch_id = base_ethdev->data->switch_id;
>>
>> +	/** Set device flags to specify that device is a representor port */
>> +	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
> 
> Should be set in the PMD, not in ethdev layer

As in the previous patch this is just a generic port bus init function 
which meets the simplest use case of representor port with a single 
switch domain, a PMD doesn't need to use it but having it here saves 
duplicating the same code across multiple PMD which are only supporting 
the basic mode.

> 
>> +
>>   	return 0;
>>   }
>>
>> --
>> 2.14.3
>
Shahaf Shuler April 1, 2018, 6:14 a.m. UTC | #4 
Thursday, March 29, 2018 5:53 PM, Doherty, Declan:
> On 29/03/2018 7:13 AM, Shahaf Shuler wrote:

> > Wednesday, March 28, 2018 4:54 PM, Declan Doherty:

> >> Subject: [dpdk-dev][PATCH v6 4/8] ethdev: Add port representor device

> >> flag

> >>

> >> Add new device flag to specify that ethdev port is a port representor.

> >> Extend rte_eth_dev_info structure to expose device flags to user

> >> which enable applications to discover if a port is a representor port.

> >>

> >> Signed-off-by: Declan Doherty <declan.doherty@intel.com>

> >> ---

> >>   lib/librte_ether/rte_ethdev.c             | 1 +

> >>   lib/librte_ether/rte_ethdev.h             | 9 ++++++---

> >>   lib/librte_ether/rte_ethdev_representor.h | 3 +++

> >>   3 files changed, 10 insertions(+), 3 deletions(-)

> >>

> >> diff --git a/lib/librte_ether/rte_ethdev.c

> >> b/lib/librte_ether/rte_ethdev.c index c719f84a3..163246433 100644

> >> --- a/lib/librte_ether/rte_ethdev.c

> >> +++ b/lib/librte_ether/rte_ethdev.c

> >> @@ -2399,6 +2399,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct

> >> rte_eth_dev_info *dev_info)

> >>   	dev_info->nb_rx_queues = dev->data->nb_rx_queues;

> >>   	dev_info->nb_tx_queues = dev->data->nb_tx_queues;

> >>   	dev_info->switch_id = dev->data->switch_id;

> >> +	dev_info->dev_flags = dev->data->dev_flags;

> >>   }

> >>

> >>   int

> >> diff --git a/lib/librte_ether/rte_ethdev.h

> >> b/lib/librte_ether/rte_ethdev.h index dced4fc41..226acc8b1 100644

> >> --- a/lib/librte_ether/rte_ethdev.h

> >> +++ b/lib/librte_ether/rte_ethdev.h

> >> @@ -996,6 +996,7 @@ struct rte_eth_dev_info {

> >>   	const char *driver_name; /**< Device Driver name. */

> >>   	unsigned int if_index; /**< Index to bound host interface, or 0 if

> >> none.

> >>   		Use if_indextoname() to translate into an interface name. */

> >> +	uint32_t dev_flags; /**< Device flags */

> >>   	uint32_t min_rx_bufsize; /**< Minimum size of RX buffer. */

> >>   	uint32_t max_rx_pktlen; /**< Maximum configurable length of RX

> >> pkt. */

> >>   	uint16_t max_rx_queues; /**< Maximum number of RX queues. */

> @@

> >> -1229,11 +1230,13 @@ struct rte_eth_dev_owner {  };

> >>

> >>   /** Device supports link state interrupt */

> >> -#define RTE_ETH_DEV_INTR_LSC     0x0002

> >> +#define RTE_ETH_DEV_INTR_LSC		0x0002

> >>   /** Device is a bonded slave */

> >> -#define RTE_ETH_DEV_BONDED_SLAVE 0x0004

> >> +#define RTE_ETH_DEV_BONDED_SLAVE	0x0004

> >>   /** Device supports device removal interrupt */

> >> -#define RTE_ETH_DEV_INTR_RMV     0x0008

> >> +#define RTE_ETH_DEV_INTR_RMV		0x0008

> >> +/** Device is port representor */

> >> +#define RTE_ETH_DEV_REPRESENTOR		0x0010

> >

> > Maybe it is a good time to make some order here.

> > I understand the decision to use flags instead of bit-field. It is better.

> >

> > However there is a mix here of device capabilities like :

> RTE_ETH_DEV_INTR_LSC   and RTE_ETH_DEV_INTR_RMV

> > And device attributes like : RTE_ETH_DEV_BONDED_SLAVE and

> RTE_ETH_DEV_REPRESENTOR.

> > I don't think they belong together under the genetic name of dev_flags.

> >

> > Moreover, I am not sure the fact device is bonded slave should be exposed

> to the application. It should be internal to ethdev and its port iterators.

> 

> That's a good point on the bonded slave flag, I'll look at fixing that for the

> next release. I don't think changing it should effect ABI but I'll need to have a

> closer look.

> 

> Do you think that we should have a separate device attributes field, which

> the representor flag is contained in.

> 

> >

> > Finally I think representor port may need more info now (and in the

> future), for example the associated vf id.

> > For that, I think it is better it to be exposed as a dedicated struct on device

> info.

> 

> I think a switch port id should suffice for that, for SR-IOV devices it would

> map to the vf_id.


I think we need both switch_domain and vf_id. 
Because for representors, the application should know which VFs can be reached from this representor and which VF it represent. 

> 

> >

> >>

> >>   /**

> >>    * @warning

> >> diff --git a/lib/librte_ether/rte_ethdev_representor.h

> >> b/lib/librte_ether/rte_ethdev_representor.h

> >> index cbc1f2855..f3726d0ba 100644

> >> --- a/lib/librte_ether/rte_ethdev_representor.h

> >> +++ b/lib/librte_ether/rte_ethdev_representor.h

> >> @@ -22,6 +22,9 @@ eth_dev_representor_port_init(struct rte_eth_dev

> >> *ethdev, void *init_params)

> >>   	/** representor inherits the switch id of it's base device */

> >>   	ethdev->data->switch_id = base_ethdev->data->switch_id;

> >>

> >> +	/** Set device flags to specify that device is a representor port */

> >> +	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;

> >

> > Should be set in the PMD, not in ethdev layer

> 

> As in the previous patch this is just a generic port bus init function which

> meets the simplest use case of representor port with a single switch domain,

> a PMD doesn't need to use it but having it here saves duplicating the same

> code across multiple PMD which are only supporting the basic mode.

> 

> >

> >> +

> >>   	return 0;

> >>   }

> >>

> >> --

> >> 2.14.3

> >
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index c719f84a3..163246433 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2399,6 +2399,7 @@  rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
 	dev_info->nb_rx_queues = dev->data->nb_rx_queues;
 	dev_info->nb_tx_queues = dev->data->nb_tx_queues;
 	dev_info->switch_id = dev->data->switch_id;
+	dev_info->dev_flags = dev->data->dev_flags;
 }
 
 int
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index dced4fc41..226acc8b1 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -996,6 +996,7 @@  struct rte_eth_dev_info {
 	const char *driver_name; /**< Device Driver name. */
 	unsigned int if_index; /**< Index to bound host interface, or 0 if none.
 		Use if_indextoname() to translate into an interface name. */
+	uint32_t dev_flags; /**< Device flags */
 	uint32_t min_rx_bufsize; /**< Minimum size of RX buffer. */
 	uint32_t max_rx_pktlen; /**< Maximum configurable length of RX pkt. */
 	uint16_t max_rx_queues; /**< Maximum number of RX queues. */
@@ -1229,11 +1230,13 @@  struct rte_eth_dev_owner {
 };
 
 /** Device supports link state interrupt */
-#define RTE_ETH_DEV_INTR_LSC     0x0002
+#define RTE_ETH_DEV_INTR_LSC		0x0002
 /** Device is a bonded slave */
-#define RTE_ETH_DEV_BONDED_SLAVE 0x0004
+#define RTE_ETH_DEV_BONDED_SLAVE	0x0004
 /** Device supports device removal interrupt */
-#define RTE_ETH_DEV_INTR_RMV     0x0008
+#define RTE_ETH_DEV_INTR_RMV		0x0008
+/** Device is port representor */
+#define RTE_ETH_DEV_REPRESENTOR		0x0010
 
 /**
  * @warning
diff --git a/lib/librte_ether/rte_ethdev_representor.h b/lib/librte_ether/rte_ethdev_representor.h
index cbc1f2855..f3726d0ba 100644
--- a/lib/librte_ether/rte_ethdev_representor.h
+++ b/lib/librte_ether/rte_ethdev_representor.h
@@ -22,6 +22,9 @@  eth_dev_representor_port_init(struct rte_eth_dev *ethdev, void *init_params)
 	/** representor inherits the switch id of it's base device */
 	ethdev->data->switch_id = base_ethdev->data->switch_id;
 
+	/** Set device flags to specify that device is a representor port */
+	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+
 	return 0;
 }