diff mbox series

ethdev: avoid blocking telemetry callback for link status

Message ID 20210114121733.46801-1-bruce.richardson@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: avoid blocking telemetry callback for link status |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS

Commit Message

Bruce Richardson Jan. 14, 2021, 12:17 p.m. UTC 
  When querying the link status via telemetry interface, we don't want the
client to have to wait for multiple seconds for a reply. Therefore use
"rte_eth_link_get_nowait()" rather than "rte_eth_link_get()" in the
telemetry callback.

Cc: stable@dpdk.org
Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Power, Ciara Jan. 14, 2021, 3:06 p.m. UTC | #1 
Hi Bruce,

>-----Original Message-----
>From: Richardson, Bruce <bruce.richardson@intel.com>
>Sent: Thursday 14 January 2021 12:18
>To: dev@dpdk.org
>Cc: Richardson, Bruce <bruce.richardson@intel.com>; stable@dpdk.org;
>Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
><ferruh.yigit@intel.com>; Andrew Rybchenko
><andrew.rybchenko@oktetlabs.ru>; Power, Ciara <ciara.power@intel.com>;
>Wiles, Keith <keith.wiles@intel.com>
>Subject: [PATCH] ethdev: avoid blocking telemetry callback for link status
>
>When querying the link status via telemetry interface, we don't want the
>client to have to wait for multiple seconds for a reply. Therefore use
>"rte_eth_link_get_nowait()" rather than "rte_eth_link_get()" in the telemetry
>callback.
>
>Cc: stable@dpdk.org
>Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
>
>Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>---
> lib/librte_ethdev/rte_ethdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>index 17ddacc78..1f4545fe0 100644
>--- a/lib/librte_ethdev/rte_ethdev.c
>+++ b/lib/librte_ethdev/rte_ethdev.c
>@@ -5692,7 +5692,7 @@ eth_dev_handle_port_link_status(const char *cmd
>__rte_unused,
> 	if (!rte_eth_dev_is_valid_port(port_id))
> 		return -1;
>
>-	ret = rte_eth_link_get(port_id, &link);
>+	ret = rte_eth_link_get_nowait(port_id, &link);
> 	if (ret < 0)
> 		return -1;
>
>--
>2.27.0

This change looks good to me.

Acked-by: Ciara Power <ciara.power@intel.com>
Thomas Monjalon Jan. 18, 2021, 2:02 p.m. UTC | #2 
> >When querying the link status via telemetry interface, we don't want the
> >client to have to wait for multiple seconds for a reply. Therefore use
> >"rte_eth_link_get_nowait()" rather than "rte_eth_link_get()" in the telemetry
> >callback.
> >
> >Cc: stable@dpdk.org
> >Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> >
> >Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> >---
> >--- a/lib/librte_ethdev/rte_ethdev.c
> >+++ b/lib/librte_ethdev/rte_ethdev.c
> >@@ -5692,7 +5692,7 @@ eth_dev_handle_port_link_status(const char *cmd
> >-	ret = rte_eth_link_get(port_id, &link);
> >+	ret = rte_eth_link_get_nowait(port_id, &link);
> 
> This change looks good to me.
> 
> Acked-by: Ciara Power <ciara.power@intel.com>

Acked-by: Thomas Monjalon <thomas@monjalon.net>
Ferruh Yigit Jan. 18, 2021, 2:48 p.m. UTC | #3 
On 1/18/2021 2:02 PM, Thomas Monjalon wrote:
>>> When querying the link status via telemetry interface, we don't want the
>>> client to have to wait for multiple seconds for a reply. Therefore use
>>> "rte_eth_link_get_nowait()" rather than "rte_eth_link_get()" in the telemetry
>>> callback.
>>>
>>> Cc: stable@dpdk.org
>>> Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
>>>
>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -5692,7 +5692,7 @@ eth_dev_handle_port_link_status(const char *cmd
>>> -	ret = rte_eth_link_get(port_id, &link);
>>> +	ret = rte_eth_link_get_nowait(port_id, &link);
>>
>> This change looks good to me.
>>
>> Acked-by: Ciara Power <ciara.power@intel.com>
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 

Applied to dpdk-next-net/main, thanks.
humin (Q) Jan. 19, 2021, 1:06 a.m. UTC | #4 
Hi, Bruce and all,
	Do you know the difference between "rte_eth_link_get" and
"rte_eth_link_get_nowait"? I know they call funciton "link_update"
with differenct parameter "wait_to_complete"(set 1 means wait, set 0
  means not wait). But how to define the "wait" time, and why it shoud wait?
	On the further, What are the application scenarios of the two
APIs?

	Look forward to your reply, thanks.

在 2021/1/14 20:17, Bruce Richardson 写道:
> When querying the link status via telemetry interface, we don't want the
> client to have to wait for multiple seconds for a reply. Therefore use
> "rte_eth_link_get_nowait()" rather than "rte_eth_link_get()" in the
> telemetry callback.
> 
> Cc: stable@dpdk.org
> Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 17ddacc78..1f4545fe0 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -5692,7 +5692,7 @@ eth_dev_handle_port_link_status(const char *cmd __rte_unused,
>   	if (!rte_eth_dev_is_valid_port(port_id))
>   		return -1;
>   
> -	ret = rte_eth_link_get(port_id, &link);
> +	ret = rte_eth_link_get_nowait(port_id, &link);
>   	if (ret < 0)
>   		return -1;
>   
>
Stephen Hemminger Jan. 19, 2021, 2:26 a.m. UTC | #5 
On Tue, 19 Jan 2021 09:06:48 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> Hi, Bruce and all,
> 	Do you know the difference between "rte_eth_link_get" and
> "rte_eth_link_get_nowait"? I know they call funciton "link_update"
> with differenct parameter "wait_to_complete"(set 1 means wait, set 0
>   means not wait). But how to define the "wait" time, and why it shoud wait?
> 	On the further, What are the application scenarios of the two
> APIs?
> 

The default behavior of rte_eth_link_get (in some drivers) is to wait
for link to come up.  Many drivers don't do this. It seems mostly the
Intel ones that do.
humin (Q) Jan. 19, 2021, 2:58 a.m. UTC | #6 
Thank Stephen,
but in which the scenarios, it should wait link to up, in which
scenarios, it should not ?
By the way, how to define the "wait" time value ?



在 2021/1/19 10:26, Stephen Hemminger 写道:
> On Tue, 19 Jan 2021 09:06:48 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
> 
>> Hi, Bruce and all,
>> 	Do you know the difference between "rte_eth_link_get" and
>> "rte_eth_link_get_nowait"? I know they call funciton "link_update"
>> with differenct parameter "wait_to_complete"(set 1 means wait, set 0
>>    means not wait). But how to define the "wait" time, and why it shoud wait?
>> 	On the further, What are the application scenarios of the two
>> APIs?
>>
> 
> The default behavior of rte_eth_link_get (in some drivers) is to wait
> for link to come up.  Many drivers don't do this. It seems mostly the
> Intel ones that do.
> .
>
Bruce Richardson Jan. 19, 2021, 10:06 a.m. UTC | #7 
On Tue, Jan 19, 2021 at 10:58:42AM +0800, Min Hu (Connor) wrote:
> Thank Stephen,
> but in which the scenarios, it should wait link to up, in which
> scenarios, it should not ?
> By the way, how to define the "wait" time value ?
> 

I believe the documentation somewhere refers to a wait time of up to 9
seconds, but I don't think it's terribly well defined. Some Intel NICs will
stall for a few seconds if the link is not yet stable or if the link
is down. With the link up, I don't think any NIC fails to return
immediately.
However, if delays in the app are something you want to avoid, the _nowait
varient is the one to call to be sure.

/Bruce

> 
> 
> 在 2021/1/19 10:26, Stephen Hemminger 写道:
> > On Tue, 19 Jan 2021 09:06:48 +0800
> > "Min Hu (Connor)" <humin29@huawei.com> wrote:
> > 
> > > Hi, Bruce and all,
> > > 	Do you know the difference between "rte_eth_link_get" and
> > > "rte_eth_link_get_nowait"? I know they call funciton "link_update"
> > > with differenct parameter "wait_to_complete"(set 1 means wait, set 0
> > >    means not wait). But how to define the "wait" time, and why it shoud wait?
> > > 	On the further, What are the application scenarios of the two
> > > APIs?
> > > 
> > 
> > The default behavior of rte_eth_link_get (in some drivers) is to wait
> > for link to come up.  Many drivers don't do this. It seems mostly the
> > Intel ones that do.
> > .
> >
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 17ddacc78..1f4545fe0 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -5692,7 +5692,7 @@  eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
-	ret = rte_eth_link_get(port_id, &link);
+	ret = rte_eth_link_get_nowait(port_id, &link);
 	if (ret < 0)
 		return -1;