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 A61C01B0F8 for ; Tue, 23 Oct 2018 09:28:45 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 092BC21FC1; Tue, 23 Oct 2018 03:28:45 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Tue, 23 Oct 2018 03:28:45 -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=KHEH4iMDZJ6hEeOYPrnUnvcVkoHkTWyJlUzFcdxping=; b=f81uY9tWFkdl 6owzlm8I4PNM6SaWx3vo94p7EzCZOtyMHOF7JetLLvCE4/TyCKyQAJR8icecppol VpR17biGzjO8whBNHuoLdPZYNISeZdgadUAS+Cpx4N14t+M/e2REMES1PjB9S7Sq AkHEGrNGsjtfe4Hp0JxhtVHysDV5K5o= 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=KHEH4iMDZJ6hEeOYPrnUnvcVkoHkTWyJlUzFcdxpi ng=; b=WpFWvWb2HB0IH5o6nM4esKkenWTbOt6qS+yLz6mlasPMsNU0Rkt4VVX/B sbdAVgGBQ3N9sAInatFULkbeAsBhVaFKBlFG05E0DxMlQDti2Zd/mQMxwhOD8HkJ RU/durYarDsalKBn8jgTvZFEKNOvS7vSXHGO6bFFmupIj1CsBPE1X03dTQ4JP+zm SkRoFebaOoPY+pXnPZ5zHZwHjkvwj6bOyyW4qvDckwZxPlzdaNoBOEqmIaWBOhTC +RFRl4+ebHCP4+md09OY9DIhfFQrD0zk3rZR9kHm7Q3j22Fa6u5jqGq6AbPqtvg/ 4+J9ANlixL5Ke70ofgFr0tTJyzb4Q== 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 DA4C0E484F; Tue, 23 Oct 2018 03:28:43 -0400 (EDT) From: Thomas Monjalon To: "Lu, Wenzhuo" Cc: "dev@dpdk.org" , "Yigit, Ferruh" , Andrew Rybchenko Date: Tue, 23 Oct 2018 09:28:45 +0200 Message-ID: <2508328.MQspLpKFhj@xps> In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09093B86DE05@shsmsx102.ccr.corp.intel.com> References: <1531373220-42150-1-git-send-email-wenzhuo.lu@intel.com> <4821031.obJuSW6AGg@xps> <6A0DE07E22DDAD4C9103DF62FEBC09093B86DE05@shsmsx102.ccr.corp.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: Tue, 23 Oct 2018 07:28:46 -0000 23/10/2018 03:25, Lu, Wenzhuo: > Hi Thomas, Ferruh, Andrew, >=20 > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > 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, > > > >>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com] > > > >>>> On 13.08.2018 05:50, Lu, Wenzhuo wrote: > > > >>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > >>>>>> 16/07/2018 03:58, Lu, Wenzhuo: > > > >>>>>>> Hi Andrew, > > > >>>>>>> From: Lu, Wenzhuo > > > >>>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com] > > > >>>>>>>>> > > > >>>>>>>>> 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, i= t should > > be documented. > > > >>>>>>>>> May be dev_info should be split into parts: part which is > > > >>>>>>>>> persistent and part which may depend on device configuratio= n. > > > >>>>>>>> As I remember, the similar discussion has happened :) I've > > > >>>>>>>> raised the similar suggestion like this. But we don=E2=80=99= 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 can= not > > 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 o= ne. > > 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 t= he > > 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=E2=80=99t familiar with others= =2E), 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 fr= om PF. > > If it not enough, it can request a larger one from PF. If the number wo= rks, 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 chan= ges. > > > > > > > >> 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 speci= fic. > > > > 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. > >=20 > > 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 a= ll > > the data into rte_eth_dev_data. > >=20 > Sorry for missing this discussion. It's a good discussion about how to op= timize the rte_eth. > But I have to say that above discussion can reach a huge reconsitution of= the rte_eth and impact every PMD. Is that fair?=20 > This patch is only try to revert a bad commit as we already find bug. As = I remember, at the beginning, Andrew said the discussion may not about the = patch but generic. So could we just tell if this patch itself OK at first? = Thanks. I have no strong opinion about your patch. Sorry, I am more interested by a possible future big clean-up. About a temporary fix, I am OK if Ferruh and Andrew are OK.