From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 9220C4C9C for ; Mon, 22 Oct 2018 14:13:08 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 0C1BB21F74; Mon, 22 Oct 2018 08:13:08 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Mon, 22 Oct 2018 08:13:08 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=Ru+VBo5UTeYxO7yfDKY3H7PeBHNAxyZAjnmYFwY94B8=; b=dgDlnviw/TAU KbO6mQLvE421d3pKCMILyIZxEg/kSiCutV77gs8EOAaMNjAvne4C6BY6Y+dRbe6O FtPeH+XYL2yAsrDG+vOz/n6Ek98QUYbo0BtmoJQRy2TIcJ+/rdmU+UUPeVWAeion aAPuhlEmYQFYsdIoWLX8CKAx89BIeek= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=Ru+VBo5UTeYxO7yfDKY3H7PeBHNAxyZAjnmYFwY94 B8=; b=K0Ku/dw49x8hrO0o+mmySGjh3Iz6xOFoeQ/xrZU5kaLSdabF3OBpUuuuo zam2sTppUuCrlRvFY3SSZTLo5I6IpAnhpReBD+154CEP3UAMi+pOCm6pXBOUViSf IBqaNUUZqymhf4r/fsHBiLtsn4f9Z5qGm6qOfs1WRhvQy003xfNfGDP6VKXKlVjr r/OSm1OIQtn8Tk3EMM1pGCSb4nW6EYtj+FV22zrDGVxMGFCrcL0eYNyq/h40Pqyy 6foJ9weAOmaEUcQrWxm9LFEuQbx5Mot4hOzNNOoJrP0oY3hKcz2RfsVmoxb0ITrH 3zJIdecLL+JLUTAI9cNIVmE4dRd5A== X-ME-Sender: X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id BB8CC102E9; Mon, 22 Oct 2018 08:13:06 -0400 (EDT) From: Thomas Monjalon To: dev@dpdk.org Cc: Ferruh Yigit , Andrew Rybchenko , "Lu, Wenzhuo" Date: Mon, 22 Oct 2018 14:13:08 +0200 Message-ID: <4821031.obJuSW6AGg@xps> In-Reply-To: References: <1531373220-42150-1-git-send-email-wenzhuo.lu@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" 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:13:09 -0000 22/10/2018 14:01, Ferruh Yigit: > 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 getti= ng > >>>>>>>> > >>>>>>>> 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 gett= ing > >>>>>>>>> > >>>>>>>>> Hi, Wenzhuo, > >>>>>>>>> > >>>>>>>>> I'm sorry, but I have more even harder questions than the previ= ous > >>>> one. > >>>>>>>>> This questions are rather generic and mainly to ethdev maintain= ers. > >>>>>>>>> > >>>>>>>>> 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 a= pp > >>>>>>>>> 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=E2=80=99t make it h= appen. > >>>>>>>> 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 inf= o 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. Yo= ur > >>>>>>> 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 cha= nging. > >>>>>>> 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 ha= ve > >>>>>>> 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 compari= ng with > >>>> handling this in rte layer, maybe we can let every NIC has its own d= ecision. > >>>>> 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_queue= s { > >>>>> 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 conf= igure, > >>>> get new value, reconfigure again, get new value and so on and stop w= hen > >>>> previous is equal to the new one. Yes, I dramatise and it sounds rea= lly bad. > >>>> In any case it would over-complicate interface and no single app wil= l do it > >>>> correctly. > >>> I think you're talking about max_rx_queues. APP can get that info be= fore 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 t= o show the value to human being. > >>> > >>> BTW, max_rx_queues could be an good example that shows that some para= meters are stable on some NICs but not on other NICs. > >>> Take Intel NICs for example (I don=E2=80=99t familiar with others.), = normally max_rx_queues is stable on PF. But on VF, as the max number is dec= ided by PF, it could be dynamic. When VF starts, it can get an default valu= e from PF. If it not enough, it can request a larger one from PF. If the nu= mber 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_conf= ig() API > >> and rte_eth_dev_info() output will be changed. > >=20 > > The example looks different to me. It is explicit changes directly > > requested by the application. So, it is not a surprise that it changes. > >=20 > >> For this patch suggested configuration changes based on some other con= fig values > >> looks ok as concept. > >> So I think we can say after every configuration related API dev info c= an be > >> changed. > >=20 > > 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. >=20 > Hi Andrew, Wenzhuo, >=20 > Back to this patch, which fixes an actual defect, >=20 > 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 c= an be > changed based on other config fields. So this reduces the scope of things= can > change in dev_info. I think we are doing too much juggling with data in ethdev layer. All these things should be the responsibility of the PMD. My radical proposal would be to remove rte_eth_dev_info and integrate all the data into rte_eth_dev_data.