From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id BD9324C9C for ; Mon, 22 Oct 2018 14:01:33 +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 orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Oct 2018 05:01:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,412,1534834800"; d="scan'208";a="97433291" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.49]) ([10.237.221.49]) by fmsmga002.fm.intel.com with ESMTP; 22 Oct 2018 05:01:30 -0700 To: Andrew Rybchenko , "Lu, Wenzhuo" , Thomas Monjalon Cc: "dev@dpdk.org" References: <1531373220-42150-1-git-send-email-wenzhuo.lu@intel.com> <6A0DE07E22DDAD4C9103DF62FEBC09093B804CD6@shsmsx102.ccr.corp.intel.com> <6A0DE07E22DDAD4C9103DF62FEBC09093B804D6C@shsmsx102.ccr.corp.intel.com> <1716659.kXphz3AMnz@xps> <6A0DE07E22DDAD4C9103DF62FEBC09093B81A090@shsmsx102.ccr.corp.intel.com> <98adeea9-f334-b0a7-599d-c73a11927ddf@solarflare.com> <6A0DE07E22DDAD4C9103DF62FEBC09093B81A5B4@shsmsx102.ccr.corp.intel.com> <567b83ad-aada-619a-32cd-24607ba4f215@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: Mon, 22 Oct 2018 13:01:30 +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] ethdev: fix device info getting 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: Mon, 22 Oct 2018 12:01:34 -0000 On 8/23/2018 9:58 AM, Andrew Rybchenko wrote: > On 22.08.2018 19:55, Ferruh Yigit wrote: >> On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote: >>> Hi Andrew, >>> >>>> -----Original Message----- >>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com] >>>> Sent: Monday, August 13, 2018 4:39 PM >>>> To: Lu, Wenzhuo ; Thomas Monjalon >>>> ; Yigit, Ferruh >>>> Cc: dev@dpdk.org >>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting >>>> >>>> On 13.08.2018 05:50, Lu, Wenzhuo wrote: >>>>> Hi Thomas, >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net] >>>>>> Sent: Wednesday, August 1, 2018 11:37 PM >>>>>> To: Lu, Wenzhuo ; Andrew Rybchenko >>>>>> ; Yigit, Ferruh >>>>>> Cc: dev@dpdk.org >>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting >>>>>> >>>>>> 16/07/2018 03:58, Lu, Wenzhuo: >>>>>>> Hi Andrew, >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo >>>>>>>> Sent: Monday, July 16, 2018 9:08 AM >>>>>>>> To: Andrew Rybchenko ; dev@dpdk.org >>>>>>>> Cc: Yigit, Ferruh ; Thomas Monjalon >>>>>>>> >>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting >>>>>>>> >>>>>>>> Hi Andrew, >>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com] >>>>>>>>> Sent: Friday, July 13, 2018 4:03 PM >>>>>>>>> To: Lu, Wenzhuo ; dev@dpdk.org >>>>>>>>> Cc: Yigit, Ferruh ; Thomas Monjalon >>>>>>>>> >>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting >>>>>>>>> >>>>>>>>> Hi, Wenzhuo, >>>>>>>>> >>>>>>>>> I'm sorry, but I have more even harder questions than the previous >>>> one. >>>>>>>>> This questions are rather generic and mainly to ethdev maintainers. >>>>>>>>> >>>>>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote: >>>>>>>>>> The device information cannot be gotten correctly before the >>>>>>>>>> configuration is set. Because on some NICs the information has >>>>>>>>>> dependence on the configuration. >>>>>>>>> Thinking about it I have the following question. Is it valid >>>>>>>>> behaviour of the dev_info if it changes after configuration? >>>>>>>>> I always thought that the primary goal of the dev_info is to >>>>>>>>> provide information to app about device capabilities to allow app >>>>>>>>> configure device and queues correctly. Now we see the case when >>>>>>>>> dev_info changes on configure. May be it is acceptable, but it is >>>>>>>>> really suspicious. If we accept it, it should be documented. >>>>>>>>> May be dev_info should be split into parts: part which is >>>>>>>>> persistent and part which may depend on device configuration. >>>>>>>> As I remember, the similar discussion has happened :) I've raised >>>>>>>> the similar suggestion like this. But we don’t make it happen. >>>>>>>> The reason is, you see, this is the rte layer's behavior. So the >>>>>>>> user doesn't have to know it. From APP's PoV, it inputs the >>>>>>>> configuration, it calls this API "rte_eth_dev_configure". It >>>>>>>> doesn't know the configuration is copied before getting the info or not. >>>>>>>> So, to my opinion, we can still keep the behavior. We only need to >>>>>>>> split it into parts when we do see the case that cannot make it. >>>>>>> Maybe I talked too much about the patch. Think about it again. Your >>>>>>> comments is about how to use the APIs, rte_eth_dev_info_get, >>>>>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to >>>>>> get the info. It can be called anywhere, before configuration or >>>>>> after. It's reasonable the info changes with the configuration changing. >>>>>>> But we do have something missing, like, rte_eth_dev_capability_get >>>>>>> which >>>>>> should be stable. APP can use this API to get the necessary info >>>>>> before configuration. >>>>>>> A question, maybe a little divergent thinking, that APP should have >>>>>>> some >>>>>> intelligence to handle the capability automatically. So getting the >>>>>> capability is not so good and effective, looks like we still need the human >>>> involvement. >>>>>> Maybe that the reason currently we suppose APP know the capability >>>>>> from the paper copies, examples... >>>>>> >>>>>> I am not sure to understand all the sentences. >>>>>> But I agree that we should take a decision about the stability of these >>>> infos. >>>>>> Either infos cannot change after probing, or we must document that >>>>>> the app must request infos regularly (when?). >>>>> Sorry, I missed this mail. >>>>> >>>>> I have the concern that different NICs have different behavior. One info >>>> can be stable on a NIC but dynamic on another. Considering this, we may >>>> better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with >>>> handling this in rte layer, maybe we can let every NIC has its own decision. >>>>> I have an idea. Maybe we can add a parameter for potential dynamic >>>>> fields. Like, Changing uint16_t nb_rx_queues; to struct nb_rx_queues { >>>>> uint16_t value; bool stable; } >>>> May be it is just very bad example, but as I understand nb_rx_queues is >>>> mainly required to configure the device properly. Or should app configure, >>>> get new value, reconfigure again, get new value and so on and stop when >>>> previous is equal to the new one. Yes, I dramatise and it sounds really bad. >>>> In any case it would over-complicate interface and no single app will do it >>>> correctly. >>> I think you're talking about max_rx_queues. APP can get that info before configuration. Then configure rx queue number which is not larger than it. That's enough. >>> nb_rx_queues should be the number which is configured by APP and how many queues are actually used. To my opinion, it's mainly used by the GUI to show the value to human being. >>> >>> BTW, max_rx_queues could be an good example that shows that some parameters are stable on some NICs but not on other NICs. >>> Take Intel NICs for example (I don’t familiar with others.), normally max_rx_queues is stable on PF. But on VF, as the max number is decided by PF, it could be dynamic. When VF starts, it can get an default value from PF. If it not enough, it can request a larger one from PF. If the number works, VF can get a new number. >> "struct rte_eth_dev_info" is a little overloaded, it has: >> - static info, like *device >> - device limitations, max_*, *_lim >> - device capabilities, *_capa >> - suggested configurations, default_*conf >> - device configuration, nb_[r/t]x_queues >> - other, switch_info >> >> There is a concern that some values are dynamic, but this is not new, for >> example nb_rx/tx_queues can be changed by rte_eth_dev_rx/tx_queue_config() API >> and rte_eth_dev_info() output will be changed. > > The example looks different to me. It is explicit changes directly > requested by the application. So, it is not a surprise that it changes. > >> For this patch suggested configuration changes based on some other config values >> looks ok as concept. >> So I think we can say after every configuration related API dev info can be >> changed. > > I think that saying that any configuration changes may result in any > changes in dev_info is hardly helpful. I'd suggest to be more specific. > Yes, it is harder and will have bugs, but at least it is helpful. Hi Andrew, Wenzhuo, Back to this patch, which fixes an actual defect, What do you think about: 1- Keep existing patch but extend it as, save the original "dev->data" and revert it back to this original data on all error path. 2- Update rte_eth_dev_info() API document and say default configuration can be changed based on other config fields. So this reduces the scope of things can change in dev_info. Thanks, ferruh > >> But rte_eth_dev_info_get() has been called within rte_eth_dev_configure() >> creating a cyclic dependency, this is forcing copy the user provided config >> before rte_eth_dev_info(). >> >> This case the concern of copying user provided config to device config in early >> stages cause inconsistent data in error case, this is valid concern and >> unfortunately already an issue with the current implementation. >> >> What would you think keep the logic in this patch but improve it with save and >> restore existing device config for error cases? >> >>>> Stable dev_info is simple. If there are real cases when something can't be >>>> stable (and may be recommended Rx/Tx ring sizes is good example, it should >>>> at least documented in dev_info structure description or may be moved to >>>> separate API. >>>> >>>>> By default, the stable is false. Then every NIC can maintain its own >>>> behavior. >>>>> Some fileds that must be stable can be left unchanged, like, driver_name, >>>> max_rx_queues. >>>>> As this patch is just reversing a bad commit to fix a bug, if my idea sounds >>>> good or worth discussing, I can send another RFC mail for it. > >