diff mbox series

[dpdk-dev,v3,27/27] net/ena: set link speed as none

Message ID 20180607094322.14312-27-mk@semihalf.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net/ena: new features and fixes |

Checks

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

Commit Message

Michal Krawczyk June 7, 2018, 9:43 a.m. UTC 
  From: Rafal Kozik <rk@semihalf.com>

Link speed should is not limited to 10Gb/s and it shouldn't be hardcoded.

They link speed is set to none instead and the applications shouldn't
rely on this value when using ENA PMD.

Fixes: 1173fca ("ena: add polling-mode driver")

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Michal Krawczyk <mk@semihalf.com>
---
 drivers/net/ena/ena_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ferruh Yigit June 8, 2018, 7:37 p.m. UTC | #1 
On 6/7/2018 10:43 AM, Michal Krawczyk wrote:
> From: Rafal Kozik <rk@semihalf.com>
> 
> Link speed should is not limited to 10Gb/s and it shouldn't be hardcoded.
> 
> They link speed is set to none instead and the applications shouldn't
> rely on this value when using ENA PMD.

Why not able to set link speed?

And what is the link_autoneg, ETH_LINK_FIXED? What is the point of setting link
speed FIXED and later speed value NONE?

> 
> Fixes: 1173fca ("ena: add polling-mode driver")

For next time can you please use git alias to have consistent Fixes format

> 
> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> Acked-by: Michal Krawczyk <mk@semihalf.com>
> ---
>  drivers/net/ena/ena_ethdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index 5c3b6494f..9ae73e331 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -848,7 +848,7 @@ static int ena_link_update(struct rte_eth_dev *dev,
>  	adapter = (struct ena_adapter *)(dev->data->dev_private);
>  
>  	link->link_status = adapter->link_status ? ETH_LINK_UP : ETH_LINK_DOWN;
> -	link->link_speed = ETH_SPEED_NUM_10G;
> +	link->link_speed = ETH_SPEED_NUM_NONE;
>  	link->link_duplex = ETH_LINK_FULL_DUPLEX;
>  
>  	return 0;
>
Chas Williams June 10, 2018, 1:35 a.m. UTC | #2 
On Fri, Jun 8, 2018 at 3:37 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 6/7/2018 10:43 AM, Michal Krawczyk wrote:
> > From: Rafal Kozik <rk@semihalf.com>
> >
> > Link speed should is not limited to 10Gb/s and it shouldn't be hardcoded.
> >
> > They link speed is set to none instead and the applications shouldn't
> > rely on this value when using ENA PMD.
>
> Why not able to set link speed?
>
> And what is the link_autoneg, ETH_LINK_FIXED? What is the point of setting
> link
> speed FIXED and later speed value NONE?
>

The link speed is part of the spanning tree path computation.  I don't
think picking 0 (or undefined)
is the appropriate choice.  For virtual interfaces link speed is a
troublesome quantity but advertising
undefined seems worse then just use some constant value.


>
> >
> > Fixes: 1173fca ("ena: add polling-mode driver")
>
> For next time can you please use git alias to have consistent Fixes format
>
> >
> > Signed-off-by: Rafal Kozik <rk@semihalf.com>
> > Acked-by: Michal Krawczyk <mk@semihalf.com>
> > ---
> >  drivers/net/ena/ena_ethdev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> > index 5c3b6494f..9ae73e331 100644
> > --- a/drivers/net/ena/ena_ethdev.c
> > +++ b/drivers/net/ena/ena_ethdev.c
> > @@ -848,7 +848,7 @@ static int ena_link_update(struct rte_eth_dev *dev,
> >       adapter = (struct ena_adapter *)(dev->data->dev_private);
> >
> >       link->link_status = adapter->link_status ? ETH_LINK_UP :
> ETH_LINK_DOWN;
> > -     link->link_speed = ETH_SPEED_NUM_10G;
> > +     link->link_speed = ETH_SPEED_NUM_NONE;
> >       link->link_duplex = ETH_LINK_FULL_DUPLEX;
> >
> >       return 0;
> >
>
>
Michal Krawczyk June 11, 2018, 8:01 a.m. UTC | #3 
2018-06-10 3:35 GMT+02:00 Chas Williams <3chas3@gmail.com>:
>
>
> On Fri, Jun 8, 2018 at 3:37 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> On 6/7/2018 10:43 AM, Michal Krawczyk wrote:
>> > From: Rafal Kozik <rk@semihalf.com>
>> >
>> > Link speed should is not limited to 10Gb/s and it shouldn't be
>> > hardcoded.
>> >
>> > They link speed is set to none instead and the applications shouldn't
>> > rely on this value when using ENA PMD.
>>
>> Why not able to set link speed?
>>
>> And what is the link_autoneg, ETH_LINK_FIXED? What is the point of setting
>> link
>> speed FIXED and later speed value NONE?
>
>
> The link speed is part of the spanning tree path computation.  I don't think
> picking 0 (or undefined)
> is the appropriate choice.  For virtual interfaces link speed is a
> troublesome quantity but advertising
> undefined seems worse then just use some constant value.
>

Setting constant value is not making much sense in the virtualized
environment (ENA can be only used in the cloud). Link speed can be
very flexible in that case and that's why ENA isn't providing it. If
we will use constant value, what it should be? It may lead to
confusion or can result in the performance issues because some
applications are relying on this number to send max traffic.
Applications should not make assumption that the link speed is always
giving valid value and if it is not, they should try to configure rate
dynamically, instead of statically.

>>
>>
>> >
>> > Fixes: 1173fca ("ena: add polling-mode driver")
>>
>> For next time can you please use git alias to have consistent Fixes format
>>
>> >
>> > Signed-off-by: Rafal Kozik <rk@semihalf.com>
>> > Acked-by: Michal Krawczyk <mk@semihalf.com>
>> > ---
>> >  drivers/net/ena/ena_ethdev.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
>> > index 5c3b6494f..9ae73e331 100644
>> > --- a/drivers/net/ena/ena_ethdev.c
>> > +++ b/drivers/net/ena/ena_ethdev.c
>> > @@ -848,7 +848,7 @@ static int ena_link_update(struct rte_eth_dev *dev,
>> >       adapter = (struct ena_adapter *)(dev->data->dev_private);
>> >
>> >       link->link_status = adapter->link_status ? ETH_LINK_UP :
>> > ETH_LINK_DOWN;
>> > -     link->link_speed = ETH_SPEED_NUM_10G;
>> > +     link->link_speed = ETH_SPEED_NUM_NONE;
>> >       link->link_duplex = ETH_LINK_FULL_DUPLEX;
>> >
>> >       return 0;
>> >
>>
>
Chas Williams June 11, 2018, 4:15 p.m. UTC | #4 
On Mon, Jun 11, 2018 at 4:01 AM Michał Krawczyk <mk@semihalf.com> wrote:

> 2018-06-10 3:35 GMT+02:00 Chas Williams <3chas3@gmail.com>:
> >
> >
> > On Fri, Jun 8, 2018 at 3:37 PM Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> >>
> >> On 6/7/2018 10:43 AM, Michal Krawczyk wrote:
> >> > From: Rafal Kozik <rk@semihalf.com>
> >> >
> >> > Link speed should is not limited to 10Gb/s and it shouldn't be
> >> > hardcoded.
> >> >
> >> > They link speed is set to none instead and the applications shouldn't
> >> > rely on this value when using ENA PMD.
> >>
> >> Why not able to set link speed?
> >>
> >> And what is the link_autoneg, ETH_LINK_FIXED? What is the point of
> setting
> >> link
> >> speed FIXED and later speed value NONE?
> >
> >
> > The link speed is part of the spanning tree path computation.  I don't
> think
> > picking 0 (or undefined)
> > is the appropriate choice.  For virtual interfaces link speed is a
> > troublesome quantity but advertising
> > undefined seems worse then just use some constant value.
> >
>
> Setting constant value is not making much sense in the virtualized
> environment (ENA can be only used in the cloud). Link speed can be
> very flexible in that case and that's why ENA isn't providing it. If
> we will use constant value, what it should be? It may lead to
> confusion or can result in the performance issues because some
> applications are relying on this number to send max traffic.
>

What applications are using this value to determine the transmit speed?


> Applications should not make assumption that the link speed is always
> giving valid value and if it is not, they should try to configure rate
> dynamically, instead of statically.


So advertising a fixed speed of 10G should be perfectly fine.  The
application should not assume 10G is a valid value and will attempt
to dynamically configure the rate.

The link speed should never be used to determine a transmit rate
for an application because you don't know the speeds in the middle
of the network.


>
> >>
> >>
> >> >
> >> > Fixes: 1173fca ("ena: add polling-mode driver")
> >>
> >> For next time can you please use git alias to have consistent Fixes
> format
> >>
> >> >
> >> > Signed-off-by: Rafal Kozik <rk@semihalf.com>
> >> > Acked-by: Michal Krawczyk <mk@semihalf.com>
> >> > ---
> >> >  drivers/net/ena/ena_ethdev.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/net/ena/ena_ethdev.c
> b/drivers/net/ena/ena_ethdev.c
> >> > index 5c3b6494f..9ae73e331 100644
> >> > --- a/drivers/net/ena/ena_ethdev.c
> >> > +++ b/drivers/net/ena/ena_ethdev.c
> >> > @@ -848,7 +848,7 @@ static int ena_link_update(struct rte_eth_dev
> *dev,
> >> >       adapter = (struct ena_adapter *)(dev->data->dev_private);
> >> >
> >> >       link->link_status = adapter->link_status ? ETH_LINK_UP :
> >> > ETH_LINK_DOWN;
> >> > -     link->link_speed = ETH_SPEED_NUM_10G;
> >> > +     link->link_speed = ETH_SPEED_NUM_NONE;
> >> >       link->link_duplex = ETH_LINK_FULL_DUPLEX;
> >> >
> >> >       return 0;
> >> >
> >>
> >
>
Michal Krawczyk June 19, 2018, 6:32 a.m. UTC | #5 
2018-06-11 18:15 GMT+02:00 Chas Williams <3chas3@gmail.com>:
>
>
> On Mon, Jun 11, 2018 at 4:01 AM Michał Krawczyk <mk@semihalf.com> wrote:
>>
>> 2018-06-10 3:35 GMT+02:00 Chas Williams <3chas3@gmail.com>:
>> >
>> >
>> > On Fri, Jun 8, 2018 at 3:37 PM Ferruh Yigit <ferruh.yigit@intel.com>
>> > wrote:
>> >>
>> >> On 6/7/2018 10:43 AM, Michal Krawczyk wrote:
>> >> > From: Rafal Kozik <rk@semihalf.com>
>> >> >
>> >> > Link speed should is not limited to 10Gb/s and it shouldn't be
>> >> > hardcoded.
>> >> >
>> >> > They link speed is set to none instead and the applications shouldn't
>> >> > rely on this value when using ENA PMD.
>> >>
>> >> Why not able to set link speed?
>> >>
>> >> And what is the link_autoneg, ETH_LINK_FIXED? What is the point of
>> >> setting
>> >> link
>> >> speed FIXED and later speed value NONE?
>> >
>> >
>> > The link speed is part of the spanning tree path computation.  I don't
>> > think
>> > picking 0 (or undefined)
>> > is the appropriate choice.  For virtual interfaces link speed is a
>> > troublesome quantity but advertising
>> > undefined seems worse then just use some constant value.
>> >
>>
>> Setting constant value is not making much sense in the virtualized
>> environment (ENA can be only used in the cloud). Link speed can be
>> very flexible in that case and that's why ENA isn't providing it. If
>> we will use constant value, what it should be? It may lead to
>> confusion or can result in the performance issues because some
>> applications are relying on this number to send max traffic.
>
>
> What applications are using this value to determine the transmit speed?
>

The pktgen for example.

>>
>> Applications should not make assumption that the link speed is always
>> giving valid value and if it is not, they should try to configure rate
>> dynamically, instead of statically.
>
>
> So advertising a fixed speed of 10G should be perfectly fine.  The
> application should not assume 10G is a valid value and will attempt
> to dynamically configure the rate.
>
> The link speed should never be used to determine a transmit rate
> for an application because you don't know the speeds in the middle
> of the network.

If we have higher upper limit, which is 25G, wouldn't that make more
sense to set is as a link speed, so the user could take it as a hint
what the upper limit is? If you prefer to keep the 10G value instead,
we will probably revert this patch.

>>
>>
>> >>
>> >>
>> >> >
>> >> > Fixes: 1173fca ("ena: add polling-mode driver")
>> >>
>> >> For next time can you please use git alias to have consistent Fixes
>> >> format
>> >>
>> >> >
>> >> > Signed-off-by: Rafal Kozik <rk@semihalf.com>
>> >> > Acked-by: Michal Krawczyk <mk@semihalf.com>
>> >> > ---
>> >> >  drivers/net/ena/ena_ethdev.c | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/drivers/net/ena/ena_ethdev.c
>> >> > b/drivers/net/ena/ena_ethdev.c
>> >> > index 5c3b6494f..9ae73e331 100644
>> >> > --- a/drivers/net/ena/ena_ethdev.c
>> >> > +++ b/drivers/net/ena/ena_ethdev.c
>> >> > @@ -848,7 +848,7 @@ static int ena_link_update(struct rte_eth_dev
>> >> > *dev,
>> >> >       adapter = (struct ena_adapter *)(dev->data->dev_private);
>> >> >
>> >> >       link->link_status = adapter->link_status ? ETH_LINK_UP :
>> >> > ETH_LINK_DOWN;
>> >> > -     link->link_speed = ETH_SPEED_NUM_10G;
>> >> > +     link->link_speed = ETH_SPEED_NUM_NONE;
>> >> >       link->link_duplex = ETH_LINK_FULL_DUPLEX;
>> >> >
>> >> >       return 0;
>> >> >
>> >>
>> >
diff mbox series

Patch

diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 5c3b6494f..9ae73e331 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -848,7 +848,7 @@  static int ena_link_update(struct rte_eth_dev *dev,
 	adapter = (struct ena_adapter *)(dev->data->dev_private);
 
 	link->link_status = adapter->link_status ? ETH_LINK_UP : ETH_LINK_DOWN;
-	link->link_speed = ETH_SPEED_NUM_10G;
+	link->link_speed = ETH_SPEED_NUM_NONE;
 	link->link_duplex = ETH_LINK_FULL_DUPLEX;
 
 	return 0;