From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f178.google.com (mail-wi0-f178.google.com [209.85.212.178]) by dpdk.org (Postfix) with ESMTP id 9E83A3787 for ; Wed, 16 Sep 2015 16:42:01 +0200 (CEST) Received: by wicgb1 with SMTP id gb1so77340408wic.1 for ; Wed, 16 Sep 2015 07:42:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-type :content-disposition:in-reply-to; bh=GS9tHkbjxykz80FIv6lxPkn3BZjhC/wPZBrktKqss9k=; b=KYqeotabBU42yJHL8JFtHxl8W+p8LZ3HcrAy67UTpDcp2upmC8wc/D2eBsUN1c5BcD rmvLAd+uFToGTlrTdd+jpggKVnFkAzPXvoiFEnhaaUiiCiYMBfS8CtuDLtMzQTIenIIe KbXy0L0EvwK9OMW3bn34oCu5iU0c5U2TwdeMNlSitnYoDj8c0f7pNvaZsXqS6mUrs2fO n23wdLe9dFUJRP8U0/4XKokvN+Gki2cdTywCFndDXvj+KBRF52B89TgcLU6gMCk5BU/R 3874jur7QjIRMYrxGfrMLuqcJvPVzc6XatKl8i92S5QwFI4UU8mYmJopn4Yho+tCj6OK hLjg== X-Gm-Message-State: ALoCoQmdDgc7IXZmPxxHNAx/VI0O+6Ch0h2PywlSjJTR7v/MJRmDshlWMYvM817l0Vyxt8dsxWmg X-Received: by 10.194.116.9 with SMTP id js9mr24494464wjb.51.1442414521352; Wed, 16 Sep 2015 07:42:01 -0700 (PDT) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id fu5sm4662wic.0.2015.09.16.07.41.59 (version=TLSv1 cipher=RC4-SHA bits=128/128); Wed, 16 Sep 2015 07:42:00 -0700 (PDT) Date: Wed, 16 Sep 2015 16:41:45 +0200 From: Adrien Mazarguil To: Marc Sune Message-ID: <20150916144145.GB4924@6wind.com> Mail-Followup-To: Marc Sune , =?utf-8?B?TsOpbGlv?= Laranjeiro , Morten =?utf-8?Q?Br=C3=B8rup?= , Thomas Monjalon , dev@dpdk.org, Olga Shern References: <2699193.9riTyGPe1z@xps13> <2046894.c3eJ0QZGuc@xps13> <98CBD80474FA8B44BF855DF32C47DC358AF3B7@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC358AF3BD@smartserver.smartshare.dk> <20150915082544.GG25122@autoinstall.dev.6wind.com> <20150915100443.GA4924@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Cc: dev@dpdk.org, Morten =?utf-8?Q?Br=C3=B8rup?= Subject: Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Sep 2015 14:42:01 -0000 On Tue, Sep 15, 2015 at 11:20:03PM +0200, Marc Sune wrote: > Adrien, > > 2015-09-15 12:04 GMT+02:00 Adrien Mazarguil : [...] > > It's not so much about the way PMDs recover link information, rather about > > the amount of changes required to switch to a bit-field API for the current > > link speed with no clear advantage. > > > See below; these are trivial changes. > > > All PMDs must be modified, the initial > > set of patches isn't complete in this regard. > > > > Thanks for pointing out this. There are a couple missing. > [...] > > I think Nelio was using mlx4 as an example, all PMDs have their own > > particular method to recover it and several must perform calculations to > > get > > the final value. Using integers for this task is certainly easier than > > going > > through bit-field conversions. > > > > Drivers have their *own* way to extract the link speed from the HW, because > the way is stored is anyway HW specific. That a driver encodes their link > speed as a numeric value is just a coincidence, and the exception. I concede that most non-virtual drivers (including all Intel drivers) only perform conversions between bit-field values, for those it's just a matter of updating a macro name in a succession of if/else or switch/case statements which is indeed trivial. Exceptions are currently af_packet, bnx2x, bonding (does not care actually), enic, mlx4, null, pcap, ring, xenvirt (these last four are purely virtual and use hard-coded values, no calculations involved but still). > Specifically, and putting an example of e1000 (which you are right it is > buggy in v4, see below): [...] > Can you please tell me which exact extra conversions are needed on the PMD > side? It only needs to be fixed: [...] > Other drivers, like i40 are already ooing it correctly: Sure, I was only pointing out the extra work required to make sure all of them were behaving as expected. [...] > > Everyone agrees that a link speed bit-field is useful as an input value to > > advertise, request and allow a set of speeds. We do not agree with its > > usage > > as the current link speed which is often the result of a computation. We > > aren't talking about performance. > > > > A given link cannot be simultaneously at 10 Gbps and 1 Gbps right? Using a > > bit-field for the current link speed is confusing at best. Output values do > > not need to be included in the unified API, they are never converted back > > into enum values. > > > > Although I agree it is unlikely that this would happen, we shouldn't > anticipate what users will do, so in either approach you would need utility > functions to translate from numerical to bitmap and viceversa. Yes, obviously. > > I'm stressing again the fact that doing so would require a changes in all > > applications that use the current speed and in PMDs for no good reason. > > Well any change in the API will. This patch (v1,v2) started as speed caps > only, and now we are refactoring the link API. How much code has to be > changed or how is easier for PMDs is irrelevant IMHO. What matters is if > the API makes sense for the user. > > And for that you are probably right; it might be more comprehensible to > have "a" speed value as a result of rte_eth_get_link(), provided that we > give utility functions to go back and forth from numerical constants and > bitmap constants (both have to be defined then in rte_eth.h). I fully agree with this. Which means ETH_LINK_SPEED_* macros can be dropped (as in your patchset) and replaced with a function or a macro that simply converts their ETH_SPEED_* counterparts to integer values. That way we don't have to keep two confusing sets of macros. Probably obvious but in the reverse function, I suggest returning ETH_SPEED_UNKNOWN for invalid values instead of the nearest match. About struct rte_eth_link, Nelio intends to submit a patch to store link_speed in a uint32_t for 100+Gbps links and update a few PMDs affected by the change. Perhaps his patch should be included in your patchset? -- Adrien Mazarguil 6WIND