From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id F07BD1B508 for ; Wed, 10 Oct 2018 16:09:37 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Oct 2018 07:09:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,364,1534834800"; d="scan'208";a="93975352" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.49]) ([10.237.221.49]) by fmsmga002.fm.intel.com with ESMTP; 10 Oct 2018 07:09:36 -0700 To: Dan Gora Cc: dev@dpdk.org, Igor Ryzhov , Stephen Hemminger References: <20180911232906.18352-1-dg@adax.com> <20180919195549.5585-1-dg@adax.com> <20180919195549.5585-2-dg@adax.com> <671135e5-a666-4254-c5c6-672c3863146b@intel.com> <61731242-db6c-0c5d-bcab-e82b45e324d7@intel.com> <846eed94-2ed7-7b89-5a3e-696ec3674a26@intel.com> <7811a241-524d-21fd-4852-b63e2bba7332@intel.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= xsFNBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABzSVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+wsGVBBMBAgA/AhsDBgsJCAcDAgYVCAIJCgsE FgIDAQIeAQIXgBYhBNI2U4dCLsKE45mBx/kz60PfE2EfBQJbughWBQkHwjOGAAoJEPkz60Pf E2Eft84QAIbKWqhgqRfoiw/BbXbA1+qm2o4UgkCRQ0yJgt9QsnbpOmPKydHH0ixCliNz1J8e mRXCkMini1bTpnzp7spOjQGLeAFkNFz6BMq8YF2mVWbGEDE9WgnAxZdi0eLY7ZQnHbE6AxKL SXmpe9INb6z3ztseFt7mqje/W/6DWYIMnH3Yz9KzxujFWDcq8UCAvPkxVQXLTMpauhFgYeEx Nub5HbvhxTfUkapLwRQsSd/HbywzqZ3s/bbYMjj5JO3tgMiM9g9HOjv1G2f1dQjHi5YQiTZl 1eIIqQ3pTic6ROaiZqNmQFXPsoOOFfXF8nN2zg8kl/sSdoXWHhama5hbwwtl1vdaygQYlmdK H2ueiFh/UvT3WG3waNv2eZiEbHV8Rk52Xyn2w1G90lV0fYC6Ket1Xjoch7kjwbx793Kz/RfQ rmBY8/S4DTGn3oq3dMdQY+b6+7VMUeLMMh2CXYO9ErkOq+qNTD1IY+cBAkXnaDbQfz0zbste ZGWH74FAZ9nCpDOqbRTrBL42aMGhfOWEyeA1x7+hl6JZfabBWAuf4nnCXuorKHzBXTrf7u7p fXsKQClWRW77PF1VmzrtKNVSytQAmlCWApQIw20AarFipXmVdIjHmJPU611WoyxZPb4JTOxx 5cv9B+nr/RIB+v5dcStyHCCwO1be7nBDdCgd4F6kTQPLzsFNBFfWTL4BEACnNA29e8TarUsB L5n6eLZHXcFvVwNLVlirWOClHXf44o2KnN3ww+eBEmKVfEFo9MSuGDNHS8Zw1NiGMYxLIUgd U6gGrVVs/VrQWL82pbMk6jCj98N+BXIri+6K1z+AImz7ax7iF1kDgRAnFWU0znWWBgM2mM8Y gDjcxfXk4sCKnvf6Gjo08Ey5zmqx7dekAKU2EEp8Q1EJY3jbymLdZWRP4AFFMTS1rGMk0/tt v71NBg1GobCcbNfn9chK/jhqxYhAJqq86RdJQkt3/9x1U1Oq0vXCt4JVVHmkxePtUiuWTTt+ aYlUAsKYZsWvncExvw77x2ArYDmaK0yfjh37wp0lY7DOJHFxoyT8tyWZlLci/VMRG2Ja33xj 0CN4C1yBg+QDeV3QFxQo42iA/ykdXPUR3ezmsND3XKvVLTC4DNb3V/EZQ7jBj64+bEK0VW4G B31VP00ApNQvSoczsIOAKdk97RNbpmPw6q10ILIB+9T1xbnFYzshzGF17oC0/GENIHATx8vZ masOZoDiOZQpeneLgnFE9JfzhLTxv6wNZcc/HLXRQVTkDsQr8ERtkAoHCf1E5+b5Yr7pfnE4 YuhET746o25S53ELUYPIs49qoJsEJL34/oexMfPGyPIlrbufiNyty5jc/1MRwUlhJlJ5IOHy ZUa+6CLR7GdImusFkPJUJwARAQABwsF8BBgBAgAmAhsMFiEE0jZTh0IuwoTjmYHH+TPrQ98T YR8FAlu6CHAFCQXE7zIACgkQ+TPrQ98TYR9nXxAAqNBgkYNyGuWUuy0GwDQCbu3iiMyH1+D7 llafPcK4NYy1Z4AYuVwC9nmLaoj+ozdqS3ncRo57ncRsKEJC46nDJJZYZ5LSJVn63Y3NBF86 lxQAgjj2oyZEwaLKtKbAFsXL43jv1pUGgSvWwYtDwHITXXFQto9rZEuUDRFSx4sg9OR+Q6/6 LY+nQQ3OdHlBkflzYMPcWgDcvcTAO6yasLEUf7UcYoSWTyMYjLB4QuNlXzTswzGVMssJF/vo V8lD1eqqaSUWG3STF6GVLQOr1NLvN5+kUBiEStHFxBpgSCvYY9sNV8FS6N24CAWMBl+10W+D 2h1yiiP5dOdPcBDYKsgqDD91/sP0WdyMJkwdQJtD49f9f+lYloxHnSAxMleOpyscg1pldw+i mPaUY1bmIknLhhkqfMmjywQOXpac5LRMibAAYkcB8v7y3kwELnt8mhqqZy6LUsqcWygNbH/W K3GGt5tRpeIXeJ25x8gg5EBQ0Jnvp/IbBYQfPLtXH0Myq2QuAhk/1q2yEIbVjS+7iowEZNyE 56K63WBJxsJPB2mvmLgn98GqB4G6GufP1ndS0XDti/2K0o8rep9xoY/JDGi0n0L0tk9BHyoP Y7kaEpu7UyY3nVdRLe5H1/MnFG8hdJ97WqnPS0buYZlrbTV0nRFL/NI2VABl18vEEXvNQiO+ vM8= Message-ID: Date: Wed, 10 Oct 2018 15:09:35 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 1/5] kni: add API to set link status on kernel interface X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Oct 2018 14:09:38 -0000 On 10/3/2018 8:07 PM, Dan Gora wrote: > On Fri, Sep 28, 2018 at 5:03 AM Ferruh Yigit wrote: >> >> On 9/28/2018 12:51 AM, Dan Gora wrote: >>> On Thu, Sep 27, 2018 at 8:44 PM, Ferruh Yigit wrote: >>>>> Well, yes the link_status (link up, link down) _is_ applied to the KNI >>>>> interface. When that occurs, most people want to know what the link >>>>> speed is that the link came up at. >>>> >>>> +1 to this, people would like to know link speed of the interface. >>>> Are you printing link speed of interface? You are printing whatever user pass to >>>> API. >>> >>> There is no such thing as "link speed of the interface". The link >>> speed is the speed of the underlying Ethernet link that the interface >>> corresponds to. This is true for all other ethernet interfaces in the >>> kernel. >> >> This is an API to set link status of KNI interface, KNI interface is a virtual >> interface and no need to be backed by a physical interface at all. > > Yes, I understand that. > >> Only kni sample application uses it in a way to match a physical interface to a >> KNI interface, but please check KNI PMD where you can have multiple KNI >> interface without any physical device at all. > > Yes, I understand that.. As I pointed out previously, if there is no > physical device which corresponds to this KNI interface, the > application can: > > 1) Not use this API at all, just as they do now. This API has nothing with if KNI interface has corresponding physical device or not, all KNI users can use it. > 2) Use the API and set the state to linkup/0Mbps/autoNeg/Full. This > is perfectly reasonable and no one would be confused by this. That is the think, you are not setting anything, you are just printing "0Mbps/autoNeg/Full" and assuming/hoping the _if_ there is a corresponding physical device app sends the values of that device to API for printing. Overall I am not sure if there is a value to discuss this more, and I don't want this relatively small issue to block the patch, I will comment more to latest version. > >>>> I guess you trust to user to provide correct values there, but since only link >>>> up & down matters, what prevents user to leave other fields, like speed, just >>>> random values? >>> >>> Nothing. What prevents anyone from providing random values for >>> anything? The point of the API was to make it super simple, just: >>> >>> rte_eth_link_get_nowait(portid, &link); >>> rte_kni_update_link(p[portid]->kni[i], &link); >> >> You are only thinking about this use case. Normally the input to API should be >> verified right, for this case there is no way to verify it and vales are not >> used at all, it is just printed in API. > > In most applications it is useful to have a message printed which > shows the *change* in link status as well as the speed that the link > came up at. If you don't provide the link speed, etc information, > then the API is not really useful. Yes the application writer can > provide whatever they want for the link speed/duplex/autoneg, but so > what? Their application will have a nonsensical log message. It's > not DPDK's job to enforce sanity on everyone's application. > > And no, not every input to every API function is verified. There are > lots of examples in the DPDK where this is not the case. The > parameters should be verified to ensure that they do not cause the > application to "break", not necessarily so that they make sense > (whatever that would mean in this case). > > It was recommended that we change the rte_kni_update_link() API to > only use the link status as an input and print the log messages > outside the API in the application code. However think about what > that would entail: > > You only want to log *changes* in the link state, not log a message > when you set the same state. This means that either: > > 1) the application would have to store the previous carrier state. > 2) rte_kni_update_link could return the previous value read from the > /sys/.../carrier file. > 3) the application could read the /sys/.../carrier file before calling > rte_kni_update_link to read the previous carrier state. > > In case 1 you're obliging all users of this API to carry around this > extra state for no real reason. The API can easily read the > /sys/.../carrier file to see the previous state while it has it open, > just as it does in this patch. > > In case 2, the application can easily detect when the state changes, > but then you end up with a return value something like -1 for errors, > 0 for previous_link == down, 1 for previous_link == up. I fail to see > how this is a better API design that what has already been proposed, > but it's at least somewhat useful, even if awkward. > > In the DPDK application, you get something like: > > rte_eth_link_get_nowait(portid, &link); > prev_link = rte_kni_update_link(p[portid]->kni[i], link.link_status); > if (prev_link >= 0) > { > if (prev_link == 0 && link.link_status == 1) > printf("LINKUP: speed %d duplex %s autoneg %s\n", ....) > else if (prev_link == 1 && link.link_status == 0) > printf("LINKDOWN:\n"); > } else { > printf("Error: something went wrong...\n"); > } > > I don't really see how this is better than: > > rte_eth_link_get_nowait(portid, &link); > rte_kni_update_link(p[portid]->kni[i], &link); > > but that's my opinion... > > In case 3, you might as well not even have this API since you're > duplicating half of the same code. > > I would be willing to implement case 2, however I still think that it > is not as good a design and will unnecessarily complicate application > code compared to the current patch here. Cases 1 and 3 are > non-starters for me. Our application needs log messages only when the > link *changes* state. These log messages need to include the link > speed/duplex/autoneg info. I think that most, if not all, > applications would want that too. It's how every other physical > ethernet driver in the linux kernel works. DPDK applications which > use only KNI virtual interfaces without a physical interface are under > no obligation to use this API function. > > thanks > dan >