From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Micha=C5=82_Krawczyk?= Subject: Re: [PATCH v3 27/27] net/ena: set link speed as none Date: Tue, 19 Jun 2018 08:32:13 +0200 Message-ID: References: <20180607094322.14312-1-mk@semihalf.com> <20180607094322.14312-27-mk@semihalf.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: Ferruh Yigit , Marcin Wojtas , "Tzalik, Guy" , "Schmeilin, Evgeny" , dev@dpdk.org, "Matushevsky, Alexander" , =?UTF-8?B?UmFmYcWCIEtvemlr?= To: Chas Williams <3chas3@gmail.com> Return-path: Received: from mail-ua0-f193.google.com (mail-ua0-f193.google.com [209.85.217.193]) by dpdk.org (Postfix) with ESMTP id 5A55C1252 for ; Tue, 19 Jun 2018 08:32:14 +0200 (CEST) Received: by mail-ua0-f193.google.com with SMTP id 74-v6so12348168uav.1 for ; Mon, 18 Jun 2018 23:32:14 -0700 (PDT) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 2018-06-11 18:15 GMT+02:00 Chas Williams <3chas3@gmail.com>: > > > On Mon, Jun 11, 2018 at 4:01 AM Micha=C5=82 Krawczyk wr= ote: >> >> 2018-06-10 3:35 GMT+02:00 Chas Williams <3chas3@gmail.com>: >> > >> > >> > On Fri, Jun 8, 2018 at 3:37 PM Ferruh Yigit >> > wrote: >> >> >> >> On 6/7/2018 10:43 AM, Michal Krawczyk wrote: >> >> > From: Rafal Kozik >> >> > >> >> > 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 >> >> > Acked-by: Michal Krawczyk >> >> > --- >> >> > 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 =3D (struct ena_adapter *)(dev->data->dev_private); >> >> > >> >> > link->link_status =3D adapter->link_status ? ETH_LINK_UP : >> >> > ETH_LINK_DOWN; >> >> > - link->link_speed =3D ETH_SPEED_NUM_10G; >> >> > + link->link_speed =3D ETH_SPEED_NUM_NONE; >> >> > link->link_duplex =3D ETH_LINK_FULL_DUPLEX; >> >> > >> >> > return 0; >> >> > >> >> >> >