diff mbox series

[v5,06/10] net/ixgbe: implement power management API

Message ID d1391fa6afc98f126a847b265fa9e6059b6d1323.1602258833.git.anatoly.burakov@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v5,01/10] eal: add new x86 cpuid support for WAITPKG |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Anatoly Burakov Oct. 9, 2020, 4:02 p.m. UTC 
  From: Liang Ma <liang.j.ma@intel.com>

Implement support for the power management API by implementing a
`get_wake_addr` function that will return an address of an RX ring's
status bit.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Signed-off-by: Liang Ma <liang.j.ma@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c |  1 +
 drivers/net/ixgbe/ixgbe_rxtx.c   | 22 ++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_rxtx.h   |  2 ++
 3 files changed, 25 insertions(+)

Comments

Wang, Haiyue Oct. 12, 2020, 7:46 a.m. UTC | #1 
Hi Liang,

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Saturday, October 10, 2020 00:02
> To: dev@dpdk.org
> Cc: Ma, Liang J <liang.j.ma@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>; Hunt, David <david.hunt@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; jerinjacobk@gmail.com; Richardson, Bruce <bruce.richardson@intel.com>;
> thomas@monjalon.net; McDaniel, Timothy <timothy.mcdaniel@intel.com>; Eads, Gage <gage.eads@intel.com>;
> Macnamara, Chris <chris.macnamara@intel.com>
> Subject: [PATCH v5 06/10] net/ixgbe: implement power management API
> 
> From: Liang Ma <liang.j.ma@intel.com>
> 
> Implement support for the power management API by implementing a
> `get_wake_addr` function that will return an address of an RX ring's
> status bit.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c |  1 +
>  drivers/net/ixgbe/ixgbe_rxtx.c   | 22 ++++++++++++++++++++++
>  drivers/net/ixgbe/ixgbe_rxtx.h   |  2 ++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 0b98e210e7..30b3f416d4 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -588,6 +588,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
>  	.udp_tunnel_port_del  = ixgbe_dev_udp_tunnel_port_del,
>  	.tm_ops_get           = ixgbe_tm_ops_get,
>  	.tx_done_cleanup      = ixgbe_dev_tx_done_cleanup,
> +	.get_wake_addr        = ixgbe_get_wake_addr,
>  };
> 
>  /*
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 977ecf5137..7a9fd2aec6 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1366,6 +1366,28 @@ const uint32_t
>  		RTE_PTYPE_INNER_L3_IPV4_EXT | RTE_PTYPE_INNER_L4_UDP,
>  };
> 
> +int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
> +		uint64_t *expected, uint64_t *mask)
> +{
> +	volatile union ixgbe_adv_rx_desc *rxdp;
> +	struct ixgbe_rx_queue *rxq = rx_queue;
> +	uint16_t desc;
> +
> +	desc = rxq->rx_tail;
> +	rxdp = &rxq->rx_ring[desc];
> +	/* watch for changes in status bit */
> +	*tail_desc_addr = &rxdp->wb.upper.status_error;
> +
> +	/*
> +	 * we expect the DD bit to be set to 1 if this descriptor was already
> +	 * written to.
> +	 */
> +	*expected = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> +	*mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> +
> +	return 0;
> +}
> +

I'm wondering that whether the '.get_wake_addr' can be specific to
like 'rxq_tailq_addr_get' ? So that one day this wake up mechanism
can be applied to 'txq_tailq_addr_get' ? :-)

Also, "volatile void **tail_desc_addr, uint64_t *expected, uint64_t *mask"
can be merged into 'struct xxx' ? So that you can expand the API easily.

Just my thoughts.

Anyway, LGTM

Acked-by: Haiyue Wang <haiyue.wang@intel.com>

> --
> 2.17.1
Wang, Haiyue Oct. 12, 2020, 8:09 a.m. UTC | #2 
Hi Liang,

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Saturday, October 10, 2020 00:02
> To: dev@dpdk.org
> Cc: Ma, Liang J <liang.j.ma@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>; Hunt, David <david.hunt@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; jerinjacobk@gmail.com; Richardson, Bruce <bruce.richardson@intel.com>;
> thomas@monjalon.net; McDaniel, Timothy <timothy.mcdaniel@intel.com>; Eads, Gage <gage.eads@intel.com>;
> Macnamara, Chris <chris.macnamara@intel.com>
> Subject: [PATCH v5 06/10] net/ixgbe: implement power management API
> 
> From: Liang Ma <liang.j.ma@intel.com>
> 
> Implement support for the power management API by implementing a
> `get_wake_addr` function that will return an address of an RX ring's
> status bit.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c |  1 +
>  drivers/net/ixgbe/ixgbe_rxtx.c   | 22 ++++++++++++++++++++++
>  drivers/net/ixgbe/ixgbe_rxtx.h   |  2 ++
>  3 files changed, 25 insertions(+)
> 


> 
> +int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
> +		uint64_t *expected, uint64_t *mask)
> +{
> +	volatile union ixgbe_adv_rx_desc *rxdp;
> +	struct ixgbe_rx_queue *rxq = rx_queue;
> +	uint16_t desc;
> +
> +	desc = rxq->rx_tail;
> +	rxdp = &rxq->rx_ring[desc];
> +	/* watch for changes in status bit */
> +	*tail_desc_addr = &rxdp->wb.upper.status_error;
> +
> +	/*
> +	 * we expect the DD bit to be set to 1 if this descriptor was already
> +	 * written to.
> +	 */
> +	*expected = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> +	*mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> +

Seems have one issue about the byte endian:
Like for BIG endian:
         *expected = rte_bswap32(IXGBE_RXDADV_STAT_DD)
             !=
         *expected = rte_bswap64(IXGBE_RXDADV_STAT_DD)

And in API 'rte_power_monitor', use uint64_t type to access the wake up
data:

static inline void rte_power_monitor(const volatile void *p,
		const uint64_t expected_value, const uint64_t value_mask,
		const uint64_t tsc_timestamp)
{
	if (value_mask) {
			const uint64_t cur_value = *(const volatile uint64_t *)p;
			const uint64_t masked = cur_value & value_mask;
			/* if the masked value is already matching, abort */
			if (masked == expected_value)
				return;
		}


So that we need the wake up address type like 16/32/64b ?

> --
> 2.17.1
Anatoly Burakov Oct. 12, 2020, 9:28 a.m. UTC | #3 
On 12-Oct-20 9:09 AM, Wang, Haiyue wrote:
> Hi Liang,
> 
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Saturday, October 10, 2020 00:02
>> To: dev@dpdk.org
>> Cc: Ma, Liang J <liang.j.ma@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang, Haiyue
>> <haiyue.wang@intel.com>; Hunt, David <david.hunt@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; jerinjacobk@gmail.com; Richardson, Bruce <bruce.richardson@intel.com>;
>> thomas@monjalon.net; McDaniel, Timothy <timothy.mcdaniel@intel.com>; Eads, Gage <gage.eads@intel.com>;
>> Macnamara, Chris <chris.macnamara@intel.com>
>> Subject: [PATCH v5 06/10] net/ixgbe: implement power management API
>>
>> From: Liang Ma <liang.j.ma@intel.com>
>>
>> Implement support for the power management API by implementing a
>> `get_wake_addr` function that will return an address of an RX ring's
>> status bit.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>> ---
>>   drivers/net/ixgbe/ixgbe_ethdev.c |  1 +
>>   drivers/net/ixgbe/ixgbe_rxtx.c   | 22 ++++++++++++++++++++++
>>   drivers/net/ixgbe/ixgbe_rxtx.h   |  2 ++
>>   3 files changed, 25 insertions(+)
>>
> 
> 
>>
>> +int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
>> +uint64_t *expected, uint64_t *mask)
>> +{
>> +volatile union ixgbe_adv_rx_desc *rxdp;
>> +struct ixgbe_rx_queue *rxq = rx_queue;
>> +uint16_t desc;
>> +
>> +desc = rxq->rx_tail;
>> +rxdp = &rxq->rx_ring[desc];
>> +/* watch for changes in status bit */
>> +*tail_desc_addr = &rxdp->wb.upper.status_error;
>> +
>> +/*
>> + * we expect the DD bit to be set to 1 if this descriptor was already
>> + * written to.
>> + */
>> +*expected = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
>> +*mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
>> +
> 
> Seems have one issue about the byte endian:
> Like for BIG endian:
>           *expected = rte_bswap32(IXGBE_RXDADV_STAT_DD)
>               !=
>           *expected = rte_bswap64(IXGBE_RXDADV_STAT_DD)
> 
> And in API 'rte_power_monitor', use uint64_t type to access the wake up
> data:
> 
> static inline void rte_power_monitor(const volatile void *p,
> const uint64_t expected_value, const uint64_t value_mask,
> const uint64_t tsc_timestamp)
> {
> if (value_mask) {
> const uint64_t cur_value = *(const volatile uint64_t *)p;
> const uint64_t masked = cur_value & value_mask;
> /* if the masked value is already matching, abort */
> if (masked == expected_value)
> return;
> }
> 
> 
> So that we need the wake up address type like 16/32/64b ?

Endian differences strike again! You're right of course.

I suspect casting everything to CPU endinanness would fix it, would it not?

> 
>> --
>> 2.17.1
Anatoly Burakov Oct. 12, 2020, 9:28 a.m. UTC | #4 
On 12-Oct-20 8:46 AM, Wang, Haiyue wrote:
> Hi Liang,
> 
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Saturday, October 10, 2020 00:02
>> To: dev@dpdk.org
>> Cc: Ma, Liang J <liang.j.ma@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang, Haiyue
>> <haiyue.wang@intel.com>; Hunt, David <david.hunt@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; jerinjacobk@gmail.com; Richardson, Bruce <bruce.richardson@intel.com>;
>> thomas@monjalon.net; McDaniel, Timothy <timothy.mcdaniel@intel.com>; Eads, Gage <gage.eads@intel.com>;
>> Macnamara, Chris <chris.macnamara@intel.com>
>> Subject: [PATCH v5 06/10] net/ixgbe: implement power management API
>>
>> From: Liang Ma <liang.j.ma@intel.com>
>>
>> Implement support for the power management API by implementing a
>> `get_wake_addr` function that will return an address of an RX ring's
>> status bit.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>> ---
>>   drivers/net/ixgbe/ixgbe_ethdev.c |  1 +
>>   drivers/net/ixgbe/ixgbe_rxtx.c   | 22 ++++++++++++++++++++++
>>   drivers/net/ixgbe/ixgbe_rxtx.h   |  2 ++
>>   3 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 0b98e210e7..30b3f416d4 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -588,6 +588,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
>>   .udp_tunnel_port_del  = ixgbe_dev_udp_tunnel_port_del,
>>   .tm_ops_get           = ixgbe_tm_ops_get,
>>   .tx_done_cleanup      = ixgbe_dev_tx_done_cleanup,
>> +.get_wake_addr        = ixgbe_get_wake_addr,
>>   };
>>
>>   /*
>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
>> index 977ecf5137..7a9fd2aec6 100644
>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
>> @@ -1366,6 +1366,28 @@ const uint32_t
>>   RTE_PTYPE_INNER_L3_IPV4_EXT | RTE_PTYPE_INNER_L4_UDP,
>>   };
>>
>> +int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
>> +uint64_t *expected, uint64_t *mask)
>> +{
>> +volatile union ixgbe_adv_rx_desc *rxdp;
>> +struct ixgbe_rx_queue *rxq = rx_queue;
>> +uint16_t desc;
>> +
>> +desc = rxq->rx_tail;
>> +rxdp = &rxq->rx_ring[desc];
>> +/* watch for changes in status bit */
>> +*tail_desc_addr = &rxdp->wb.upper.status_error;
>> +
>> +/*
>> + * we expect the DD bit to be set to 1 if this descriptor was already
>> + * written to.
>> + */
>> +*expected = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
>> +*mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
>> +
>> +return 0;
>> +}
>> +
> 
> I'm wondering that whether the '.get_wake_addr' can be specific to
> like 'rxq_tailq_addr_get' ? So that one day this wake up mechanism
> can be applied to 'txq_tailq_addr_get' ? :-)
> 
> Also, "volatile void **tail_desc_addr, uint64_t *expected, uint64_t *mask"
> can be merged into 'struct xxx' ? So that you can expand the API easily.
> 
> Just my thoughts.
> 
> Anyway, LGTM
> 
> Acked-by: Haiyue Wang <haiyue.wang@intel.com>

Great point, will think of how to address it.

> 
>> --
>> 2.17.1
Anatoly Burakov Oct. 12, 2020, 9:44 a.m. UTC | #5 
On 12-Oct-20 8:46 AM, Wang, Haiyue wrote:
> Hi Liang,
> 
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Saturday, October 10, 2020 00:02
>> To: dev@dpdk.org
>> Cc: Ma, Liang J <liang.j.ma@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang, Haiyue
>> <haiyue.wang@intel.com>; Hunt, David <david.hunt@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; jerinjacobk@gmail.com; Richardson, Bruce <bruce.richardson@intel.com>;
>> thomas@monjalon.net; McDaniel, Timothy <timothy.mcdaniel@intel.com>; Eads, Gage <gage.eads@intel.com>;
>> Macnamara, Chris <chris.macnamara@intel.com>
>> Subject: [PATCH v5 06/10] net/ixgbe: implement power management API
>>
>> From: Liang Ma <liang.j.ma@intel.com>
>>
>> Implement support for the power management API by implementing a
>> `get_wake_addr` function that will return an address of an RX ring's
>> status bit.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>> ---
>>   drivers/net/ixgbe/ixgbe_ethdev.c |  1 +
>>   drivers/net/ixgbe/ixgbe_rxtx.c   | 22 ++++++++++++++++++++++
>>   drivers/net/ixgbe/ixgbe_rxtx.h   |  2 ++
>>   3 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 0b98e210e7..30b3f416d4 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -588,6 +588,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
>>   .udp_tunnel_port_del  = ixgbe_dev_udp_tunnel_port_del,
>>   .tm_ops_get           = ixgbe_tm_ops_get,
>>   .tx_done_cleanup      = ixgbe_dev_tx_done_cleanup,
>> +.get_wake_addr        = ixgbe_get_wake_addr,
>>   };
>>
>>   /*
>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
>> index 977ecf5137..7a9fd2aec6 100644
>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
>> @@ -1366,6 +1366,28 @@ const uint32_t
>>   RTE_PTYPE_INNER_L3_IPV4_EXT | RTE_PTYPE_INNER_L4_UDP,
>>   };
>>
>> +int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
>> +uint64_t *expected, uint64_t *mask)
>> +{
>> +volatile union ixgbe_adv_rx_desc *rxdp;
>> +struct ixgbe_rx_queue *rxq = rx_queue;
>> +uint16_t desc;
>> +
>> +desc = rxq->rx_tail;
>> +rxdp = &rxq->rx_ring[desc];
>> +/* watch for changes in status bit */
>> +*tail_desc_addr = &rxdp->wb.upper.status_error;
>> +
>> +/*
>> + * we expect the DD bit to be set to 1 if this descriptor was already
>> + * written to.
>> + */
>> +*expected = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
>> +*mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
>> +
>> +return 0;
>> +}
>> +
> 
> I'm wondering that whether the '.get_wake_addr' can be specific to
> like 'rxq_tailq_addr_get' ? So that one day this wake up mechanism
> can be applied to 'txq_tailq_addr_get' ? :-)

What would be the point of sleeping on TX queue though?

> 
> Also, "volatile void **tail_desc_addr, uint64_t *expected, uint64_t *mask"
> can be merged into 'struct xxx' ? So that you can expand the API easily.

Actually, i don't think we can do that. Well, we can, but we'll have to 
either define a new struct for ethdev, or define it in the power library 
and make ethdev dependent on the power library. The latter is a no-go, 
and the former i don't think is a good idea because adding a new struct 
to ethdev is big deal and i'd like to avoid that if i can.

> 
> Just my thoughts.
> 
> Anyway, LGTM
> 
> Acked-by: Haiyue Wang <haiyue.wang@intel.com>
> 
>> --
>> 2.17.1
Wang, Haiyue Oct. 12, 2020, 3:58 p.m. UTC | #6 
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Monday, October 12, 2020 17:45
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: Ma, Liang J <liang.j.ma@intel.com>; Guo, Jia <jia.guo@intel.com>; Hunt, David
> <david.hunt@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; jerinjacobk@gmail.com;
> Richardson, Bruce <bruce.richardson@intel.com>; thomas@monjalon.net; McDaniel, Timothy
> <timothy.mcdaniel@intel.com>; Eads, Gage <gage.eads@intel.com>; Macnamara, Chris
> <chris.macnamara@intel.com>
> Subject: Re: [PATCH v5 06/10] net/ixgbe: implement power management API
> 
> On 12-Oct-20 8:46 AM, Wang, Haiyue wrote:
> > Hi Liang,
> >
> >> -----Original Message-----
> >> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> >> Sent: Saturday, October 10, 2020 00:02
> >> To: dev@dpdk.org
> >> Cc: Ma, Liang J <liang.j.ma@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang, Haiyue
> >> <haiyue.wang@intel.com>; Hunt, David <david.hunt@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; jerinjacobk@gmail.com; Richardson, Bruce
> <bruce.richardson@intel.com>;
> >> thomas@monjalon.net; McDaniel, Timothy <timothy.mcdaniel@intel.com>; Eads, Gage
> <gage.eads@intel.com>;
> >> Macnamara, Chris <chris.macnamara@intel.com>
> >> Subject: [PATCH v5 06/10] net/ixgbe: implement power management API
> >>
> >> From: Liang Ma <liang.j.ma@intel.com>
> >>
> >> Implement support for the power management API by implementing a
> >> `get_wake_addr` function that will return an address of an RX ring's
> >> status bit.
> >>
> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> >> ---
> >>   drivers/net/ixgbe/ixgbe_ethdev.c |  1 +
> >>   drivers/net/ixgbe/ixgbe_rxtx.c   | 22 ++++++++++++++++++++++
> >>   drivers/net/ixgbe/ixgbe_rxtx.h   |  2 ++
> >>   3 files changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> index 0b98e210e7..30b3f416d4 100644
> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> @@ -588,6 +588,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
> >>   .udp_tunnel_port_del  = ixgbe_dev_udp_tunnel_port_del,
> >>   .tm_ops_get           = ixgbe_tm_ops_get,
> >>   .tx_done_cleanup      = ixgbe_dev_tx_done_cleanup,
> >> +.get_wake_addr        = ixgbe_get_wake_addr,
> >>   };
> >>
> >>   /*
> >> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> >> index 977ecf5137..7a9fd2aec6 100644
> >> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> >> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> >> @@ -1366,6 +1366,28 @@ const uint32_t
> >>   RTE_PTYPE_INNER_L3_IPV4_EXT | RTE_PTYPE_INNER_L4_UDP,
> >>   };
> >>
> >> +int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
> >> +uint64_t *expected, uint64_t *mask)
> >> +{
> >> +volatile union ixgbe_adv_rx_desc *rxdp;
> >> +struct ixgbe_rx_queue *rxq = rx_queue;
> >> +uint16_t desc;
> >> +
> >> +desc = rxq->rx_tail;
> >> +rxdp = &rxq->rx_ring[desc];
> >> +/* watch for changes in status bit */
> >> +*tail_desc_addr = &rxdp->wb.upper.status_error;
> >> +
> >> +/*
> >> + * we expect the DD bit to be set to 1 if this descriptor was already
> >> + * written to.
> >> + */
> >> +*expected = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> >> +*mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> >> +
> >> +return 0;
> >> +}
> >> +
> >
> > I'm wondering that whether the '.get_wake_addr' can be specific to
> > like 'rxq_tailq_addr_get' ? So that one day this wake up mechanism
> > can be applied to 'txq_tailq_addr_get' ? :-)
> 
> What would be the point of sleeping on TX queue though?

I checked, seems that the PMD uses internal index, no address, please ignore
this bad idea. ;-)

> 
> >
> > Also, "volatile void **tail_desc_addr, uint64_t *expected, uint64_t *mask"
> > can be merged into 'struct xxx' ? So that you can expand the API easily.
> 
> Actually, i don't think we can do that. Well, we can, but we'll have to
> either define a new struct for ethdev, or define it in the power library
> and make ethdev dependent on the power library. The latter is a no-go,
> and the former i don't think is a good idea because adding a new struct
> to ethdev is big deal and i'd like to avoid that if i can.

Understood the design now, thanks!

> 
> >
> > Just my thoughts.
> >
> > Anyway, LGTM
> >
> > Acked-by: Haiyue Wang <haiyue.wang@intel.com>
> >
> >> --
> >> 2.17.1
> 
> 
> --
> Thanks,
> Anatoly
Wang, Haiyue Oct. 13, 2020, 1:17 a.m. UTC | #7 
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Monday, October 12, 2020 17:29
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: Ma, Liang J <liang.j.ma@intel.com>; Guo, Jia <jia.guo@intel.com>; Hunt, David
> <david.hunt@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; jerinjacobk@gmail.com;
> Richardson, Bruce <bruce.richardson@intel.com>; thomas@monjalon.net; McDaniel, Timothy
> <timothy.mcdaniel@intel.com>; Eads, Gage <gage.eads@intel.com>; Macnamara, Chris
> <chris.macnamara@intel.com>
> Subject: Re: [PATCH v5 06/10] net/ixgbe: implement power management API
> 
> On 12-Oct-20 9:09 AM, Wang, Haiyue wrote:
> > Hi Liang,
> >
> >> -----Original Message-----
> >> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> >> Sent: Saturday, October 10, 2020 00:02
> >> To: dev@dpdk.org
> >> Cc: Ma, Liang J <liang.j.ma@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang, Haiyue
> >> <haiyue.wang@intel.com>; Hunt, David <david.hunt@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; jerinjacobk@gmail.com; Richardson, Bruce
> <bruce.richardson@intel.com>;
> >> thomas@monjalon.net; McDaniel, Timothy <timothy.mcdaniel@intel.com>; Eads, Gage
> <gage.eads@intel.com>;
> >> Macnamara, Chris <chris.macnamara@intel.com>
> >> Subject: [PATCH v5 06/10] net/ixgbe: implement power management API
> >>
> >> From: Liang Ma <liang.j.ma@intel.com>
> >>
> >> Implement support for the power management API by implementing a
> >> `get_wake_addr` function that will return an address of an RX ring's
> >> status bit.
> >>
> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> >> ---
> >>   drivers/net/ixgbe/ixgbe_ethdev.c |  1 +
> >>   drivers/net/ixgbe/ixgbe_rxtx.c   | 22 ++++++++++++++++++++++
> >>   drivers/net/ixgbe/ixgbe_rxtx.h   |  2 ++
> >>   3 files changed, 25 insertions(+)
> >>
> >
> >
> >>
> >> +int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
> >> +uint64_t *expected, uint64_t *mask)
> >> +{
> >> +volatile union ixgbe_adv_rx_desc *rxdp;
> >> +struct ixgbe_rx_queue *rxq = rx_queue;
> >> +uint16_t desc;
> >> +
> >> +desc = rxq->rx_tail;
> >> +rxdp = &rxq->rx_ring[desc];
> >> +/* watch for changes in status bit */
> >> +*tail_desc_addr = &rxdp->wb.upper.status_error;
> >> +
> >> +/*
> >> + * we expect the DD bit to be set to 1 if this descriptor was already
> >> + * written to.
> >> + */
> >> +*expected = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> >> +*mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> >> +
> >
> > Seems have one issue about the byte endian:
> > Like for BIG endian:
> >           *expected = rte_bswap32(IXGBE_RXDADV_STAT_DD)
> >               !=
> >           *expected = rte_bswap64(IXGBE_RXDADV_STAT_DD)
> >
> > And in API 'rte_power_monitor', use uint64_t type to access the wake up
> > data:
> >
> > static inline void rte_power_monitor(const volatile void *p,
> > const uint64_t expected_value, const uint64_t value_mask,
> > const uint64_t tsc_timestamp)
> > {
> > if (value_mask) {
> > const uint64_t cur_value = *(const volatile uint64_t *)p;
> > const uint64_t masked = cur_value & value_mask;
> > /* if the masked value is already matching, abort */
> > if (masked == expected_value)
> > return;
> > }
> >
> >
> > So that we need the wake up address type like 16/32/64b ?
> 
> Endian differences strike again! You're right of course.
> 
> I suspect casting everything to CPU endinanness would fix it, would it not?

But need the same date type, if swap is needed for casting, then
(u64 a = rte_bswap32(1)) != (u64 b = rte_bswap64(1))

> 
> >
> >> --
> >> 2.17.1
> 
> 
> --
> Thanks,
> Anatoly
diff mbox series

Patch

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 0b98e210e7..30b3f416d4 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -588,6 +588,7 @@  static const struct eth_dev_ops ixgbe_eth_dev_ops = {
 	.udp_tunnel_port_del  = ixgbe_dev_udp_tunnel_port_del,
 	.tm_ops_get           = ixgbe_tm_ops_get,
 	.tx_done_cleanup      = ixgbe_dev_tx_done_cleanup,
+	.get_wake_addr        = ixgbe_get_wake_addr,
 };
 
 /*
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 977ecf5137..7a9fd2aec6 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -1366,6 +1366,28 @@  const uint32_t
 		RTE_PTYPE_INNER_L3_IPV4_EXT | RTE_PTYPE_INNER_L4_UDP,
 };
 
+int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
+		uint64_t *expected, uint64_t *mask)
+{
+	volatile union ixgbe_adv_rx_desc *rxdp;
+	struct ixgbe_rx_queue *rxq = rx_queue;
+	uint16_t desc;
+
+	desc = rxq->rx_tail;
+	rxdp = &rxq->rx_ring[desc];
+	/* watch for changes in status bit */
+	*tail_desc_addr = &rxdp->wb.upper.status_error;
+
+	/*
+	 * we expect the DD bit to be set to 1 if this descriptor was already
+	 * written to.
+	 */
+	*expected = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
+	*mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
+
+	return 0;
+}
+
 /* @note: fix ixgbe_dev_supported_ptypes_get() if any change here. */
 static inline uint32_t
 ixgbe_rxd_pkt_info_to_pkt_type(uint32_t pkt_info, uint16_t ptype_mask)
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index 7e09291b22..75020fa2fc 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -299,5 +299,7 @@  uint64_t ixgbe_get_tx_port_offloads(struct rte_eth_dev *dev);
 uint64_t ixgbe_get_rx_queue_offloads(struct rte_eth_dev *dev);
 uint64_t ixgbe_get_rx_port_offloads(struct rte_eth_dev *dev);
 uint64_t ixgbe_get_tx_queue_offloads(struct rte_eth_dev *dev);
+int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
+		uint64_t *expected, uint64_t *mask);
 
 #endif /* _IXGBE_RXTX_H_ */