From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from m15-45.126.com (m15-45.126.com [220.181.15.45]) by dpdk.org (Postfix) with ESMTP id 6A61358DD for ; Tue, 20 Sep 2016 12:47:55 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=126.com; s=s110527; h=Date:From:Subject:MIME-Version:Message-ID; bh=RFIDs ZFpm44LHP0rob/js0P+G5vn/YZ8xJ1VbYl9xKc=; b=OaUb1AXzGVdGDFFzirjUZ k7lLuhE/H8GGECtT+1q1QL2k5pjJTro5w4AJy4NNzuCh2YyRZfyxzqQsgpXpEQvV oqDU2uHlaBFm5qupDBNjpfncOyy91sBHQr1KEsWdfh3yA/9osf9LKdOf327Il6+x 3knJF7e45GcLJZcDmi3+ok= Received: from zhangwqh$126.com ( [69.138.24.88, 54.215.2.217, 10.144.1.72] ) by ajax-webmail-wmsvr45 (Coremail) ; Tue, 20 Sep 2016 18:47:50 +0800 (CST) X-Originating-IP: [69.138.24.88, 54.215.2.217, 10.144.1.72] Date: Tue, 20 Sep 2016 18:47:50 +0800 (CST) From: =?GBK?B?1cXOsA==?= To: "Andriy Berestovskyy" Cc: "dev@dpdk.org" , "Matthew Hall" , nikita@gandi.net X-Priority: 3 X-Mailer: Coremail Webmail Server Version SP_ntes V3.5 build 20160420(83524.8626) Copyright (c) 2002-2016 www.mailtech.cn 126com In-Reply-To: References: <365e4837.62d.157448762d2.Coremail.zhangwqh@126.com> X-CM-CTRLDATA: 5rUmQGZvb3Rlcl9odG09MjI3Mzo1Ng== MIME-Version: 1.0 Message-ID: <356cd0e7.bdfb.15747357ddf.Coremail.zhangwqh@126.com> X-Coremail-Locale: zh_CN X-CM-TRANSID: LcqowABnb0PXE+FXZhc4AA--.2385W X-CM-SenderInfo: x2kd0wxztkqiyswou0bp/1tbi7g7q6VWnkl9dZgABsm X-Coremail-Antispam: 1U5529EdanIXcx71UUUUU7vcSsGvfC2KfnxnUU== Content-Type: text/plain; charset=GBK Content-Transfer-Encoding: base64 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] lpm performance 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: Tue, 20 Sep 2016 10:47:56 -0000 VGhhbmtzIHNvIG11Y2ggZm9yIHlvdXIgcmVwbHkhICBVc3VhbGx5IGhvdyBkaWQgeW91IHRlc3Qg bHBtIHBlcmZvcm1hbmNlIHdpdGggdmFyaWV0eSBvZiBkZXN0aW5hdGlvbiBhZGRyZXNzZXM/IHVz ZSB3aGljaCB0b29sIHNlbmQgdGhlIHRyYWZmaWM/IGhvdyBtYW55IGZsb3dzIHJ1bGVzIHdpbGwg eW91IGFkZD8gd2hhdCdzIHRoZSBwZXJmb3JtYW5jZSB5b3UgZ2V0PwoKCgoKCgoKCkF0IDIwMTYt MDktMjAgMTc6NDE6MTMsICJBbmRyaXkgQmVyZXN0b3Zza3l5IiA8YWJlckBzZW1paGFsZi5jb20+ IHdyb3RlOgo+SGV5LAo+WW91IGFyZSBjb3JyZWN0LiBUaGUgTFBNIG1pZ2h0IG5lZWQganVzdCBv bmUgKFRCTDI0KSBvciB0d28gbWVtb3J5Cj5yZWFkcyAoVEJMMjQgKyBUQkw4KS4gVGhlIHBlcmZv cm1hbmNlIGFsc28gZHJvcHMgb25jZSB5b3UgaGF2ZSBhCj52YXJpZXR5IG9mIGRlc3RpbmF0aW9u IGFkZHJlc3NlcyBpbnN0ZWFkIG9mIGp1c3Qgb25lIChjYWNoZSBtaXNzZXMpLgo+Cj5JbiB5b3Vy IGNhc2UgZm9yIHRoZSBkc3QgSVAgMTkyLjE2OC4xLjIgeW91IHdpbGwgaGF2ZSB0d28gbWVtb3J5 IHJlYWRzCj4oVEJMMjQgKyBUQkw4KSwgYmVjYXVzZSAxOTIuMTY4LjEvMjQgYmxvY2sgaGFzIHRo ZSBtb3JlIHNwZWNpZmljIHJvdXRlCj4xOTIuMTY4LjEuMS8zMi4KPgo+UmVnYXJkcywKPkFuZHJp eQo+Cj5PbiBUdWUsIFNlcCAyMCwgMjAxNiBhdCAxMjoxOCBBTSwg1cXOsCA8emhhbmd3cWhAMTI2 LmNvbT4gd3JvdGU6Cj4+IEhpIGFsbCwKPj4KPj4KPj4gRG9lcyBhbnlvbmUgdGVzdCBJUHY0IHBl cmZvcm1hbmNlPyBJZiBzbywgd2hhdCdzIHRoZSB0aHJvdWdocHV0PyBJIGNhbiBnZXQgYWxtb3N0 IDEwR2Igd2l0aCA2NCBieXRlIHBhY2tldHMuICBCdXQgYmVmb3JlIHRoZSB0ZXN0LCBJIHdvdWxk IGV4cGVjdCBpdCB3aWxsIGJlIGxlc3MgdGhhbiAxMEcuICBJIHRob3VnaHQgdGhlIHBlcmZvcm1h bmNlIHdpbGwgbm90IGJlIGFmZmVjdGVkIGJ5IHRoZSAgbnVtYmVyIG9mIHJ1bGUgZW50aXJlcy4g QnV0IHRoZSB0aHJvdWdocHV0IHdpbGwgYmUgcmVsYXRlZCB0byB3aGV0aGVyIHRoZSBmbG93IG5l ZWRzIHRvIGNoZWNrIHRoZSBzZWNvbmQgbGF5ZXIgdGFibGUgOiBUQkw4LiAgSXMgbXkgdW5kZXJz dGFuZGluZyBjb3JyZWN0PyBJIGFkZGVkIHRoaXMgZmxvdyBlbnRyaWVzIGZvbGxvd2luZyB0aGlz IGxpbms6Cj4+IGh0dHA6Ly93d3cuc2xpZGVzaGFyZS5uZXQvZ2FyeWFjaHkvdW5kZXJzdGFuZGlu Zy1kZHBkLWFsZ29yaXRobWljcwo+PiBzbGlkZSAxMCwKPj4KPj4KPj4KPj4gc3RydWN0IGlwdjRf bHBtX3JvdXRlIGlwdjRfbHBtX3JvdXRlX2FycmF5W10gPSB7Cj4+Cj4+ICAgICAgICAge0lQdjQo MTkyLCAxNjgsIDAsIDApLCAxNiwgMH0sCj4+Cj4+ICAgICAgICAge0lQdjQoMTkyLCAxNjgsIDEs IDApLCAyNCwgMX0sCj4+Cj4+ICAgICAgICAge0lQdjQoMTkyLCAxNjgsIDEsIDEpLCAzMiwgMn0K Pj4KPj4gfTsKPj4KPj4gc2VuZCB0aGUgZmxvdyB3aXRoIGRzdCBJUDoKPj4KPj4gMTkyLjE2OC4x LjIKPj4KPj4gSXQgc2hvdWxkIGNoZWNrIHRoZSBzZWNvbmQgbGF5ZXIgdGFibGUuIEJ1dCB0aGUg cGVyZm9ybWFuY2UgaXMgc3RpbGwgMTBHLiAgRG9lcyBhbnkgcGFydCBnbyB3cm9uZyB3aXRoIG15 IHNldHVwPyBPciBpdCByZWFsbHkgY2FuIGFjaGlldmUgMTBHIHdpdGggNjQgYnl0ZSBwYWNrZXQg c2l6ZS4KPj4KPj4gVGhhbmtzLAo+Pgo+Pgo+Cj4KPgo+LS0gCj5BbmRyaXkgQmVyZXN0b3Zza3l5 Cg== >From viktorin@rehivetech.com Tue Sep 20 12:51:58 2016 Return-Path: Received: from wes1-so2.wedos.net (wes1-so2.wedos.net [46.28.106.16]) by dpdk.org (Postfix) with ESMTP id 74A38690F for ; Tue, 20 Sep 2016 12:51:58 +0200 (CEST) Received: from pcviktorin.fit.vutbr.cz (pcviktorin.fit.vutbr.cz [147.229.13.147]) by wes1-so2.wedos.net (Postfix) with ESMTPSA id 3sdfhY6s9yz7RK; Tue, 20 Sep 2016 12:51:57 +0200 (CEST) Date: Tue, 20 Sep 2016 12:49:06 +0200 From: Jan Viktorin To: Shreyansh Jain Cc: "dev@dpdk.org" , Hemant Agrawal , David Marchand Message-ID: <20160920124906.64229000@pcviktorin.fit.vutbr.cz> In-Reply-To: References: <1451682326-5834-1-git-send-email-viktorin@rehivetech.com> <1473410639-10367-1-git-send-email-shreyansh.jain@nxp.com> <1473410639-10367-7-git-send-email-shreyansh.jain@nxp.com> <20160916142703.607722e7@pcviktorin.fit.vutbr.cz> <20160919133404.0f7fad3e@pcviktorin.fit.vutbr.cz> Organization: RehiveTech MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 06/15] eal/soc: implement probing of drivers 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: Tue, 20 Sep 2016 10:51:58 -0000 On Tue, 20 Sep 2016 06:46:31 +0000 Shreyansh Jain wrote: > Hi Jan, > > > -----Original Message----- > > From: Jan Viktorin [mailto:viktorin@rehivetech.com] > > Sent: Monday, September 19, 2016 5:04 PM > > To: Shreyansh Jain > > Cc: dev@dpdk.org; Hemant Agrawal > > Subject: Re: [PATCH v3 06/15] eal/soc: implement probing of drivers > > > > On Mon, 19 Sep 2016 12:17:53 +0530 > > Shreyansh Jain wrote: > > > > > Hi Jan, [...] > > > > > > The model for the patchset was to allow PMDs to write their own match > > > and hence, verifying a particular match is not definitive. Rather, the > > > > If you want to verify a particular match implementation then there should > > be a particular test verifying that implementation, e.g. > > test_match_compatible(), > > test_match_proprietary, test_match_by_name. > > > > However, this is testing the rte_eal_soc_probe (at least, I understand it > > that way). > > The probe iterates over devices and drivers and matches them. Thus, the > > argument > > "a particular match is not definitive" seems to be irrelevant here. You > > should build > > a testing match function like "match_always" that verifies the probe is > > working. Not > > that the "match" is working. > > Ok. 'match_always' called after 'always_find_device0' like scan function. So, essentially rather than a functional match, the testcase only checks if these handlers can be called or not. The naming of such handlers in test case would explain the user what the intention of the test is, rather than its outcome. Is this what you are suggesting? Yes, it seems to be ;). > > > > > > test case simply confirms that a SoC based PMD would be able to > > > > It does not confirm anything from my point of view. You *always* print > > "successful" > > at the end of this test (see below). > > > > > implement its own match/scan and these would be called from EAL as > > expected. > > > > > > > > > > >> +{ > > > >> + struct rte_soc_driver *drv; > > > >> + > > > >> + /* Registering dummy drivers */ > > > >> + rte_eal_soc_register(&empty_pmd0.soc_drv); > > > >> + rte_eal_soc_register(&empty_pmd1.soc_drv); > > > >> + /* Assuming that test_register_unregister is working, not > > verifying > > > >> + * that drivers are indeed registered > > > >> + */ > > > >> + > > > >> + /* rte_eal_soc_init is called by rte_eal_init, which in turn > > calls the > > > >> + * scan_fn of each driver. > > > > So, I'd comment this as something like: > > > > "mimic rte_eal_soc_init to prepare for the rte_eal_soc_probe" > > Agree. > > > > > > >> + */ > > > >> + TAILQ_FOREACH(drv, &soc_driver_list, next) { > > > >> + if (drv && drv->scan_fn) > > > >> + drv->scan_fn(); > > > >> + } > > > > > > > > Here, I suppose you mimic the rte_eal_soc_init? > > > > > > Yes. > > > > > > > > > > >> + > > > >> + /* rte_eal_init() would perform other inits here */ > > > >> + > > > >> + /* Probe would link the SoC devices<=>drivers */ > > > >> + rte_eal_soc_probe(); > > > >> + > > > >> + /* Unregistering dummy drivers */ > > > >> + rte_eal_soc_unregister(&empty_pmd0.soc_drv); > > > >> + rte_eal_soc_unregister(&empty_pmd1.soc_drv); > > > >> + > > > >> + free(empty_pmd0.soc_dev.addr.name); > > > >> + > > > >> + printf("%s has been successful\n", __func__); > > > > > > > > How you detect it is unsuccessful? Is it possible to fail in this test? > > > > A test that can never fail is in fact not a test :). > > > > > > The design assumption for SoC patcheset was: A PMDs scan is called to > > > find devices on its bus (PMD ~ bus). Whether devices are found or not, > > > is irrelevant to EAL - whether that is because of error or actually no > > > devices were available. > > > With the above logic, no 'success/failure' is checked in the test. It is > > > simply a verification of EAL's ability to link the PMD with it > > > (scan/match function pointers). > > > > I am sorry, I disagree. You always print "successful". The only way to fail > > here is a SIGSEGV or other very serious system failure. But we test > > rte_eal_soc_probe > > and not system failures. > > Ok. I am not yet clear how the test case would be created, except what I have mentioned above that rather than being functional, testcase only explains the case through function naming. > The premise on which match/scan are based is that they would be implemented by the respective PMD. Which makes testing of such function either irrelevant or simply a help to user to understand how SoC PMD should be created. > If this persists, probably I would rather remove this test case all together. It doesn't serve any purpose, which you have rightly pointed out. I'd vote to have this test to check for regressions. See the test_pci.c, e.g. test_pci_blacklist(). It is not ideal but it works. You can check how many devices have been matched or seen. Unfortunately, I am not familiar with DPAA2 API. I assume that it works via /sys as well as platform_device but with different properties. You can create a fake /sys sub-tree (see app/test/test_pci_sysfs/) and test whether your scan function reads data successfully and fills all the structures as expected. I think, there is a lot of space in this area. Next, is it possible to write a generic DPAA2 backend for the SoC Framework? I mean to not introduce the DPAA2 into PMDs (this looks like mixing of responsibilities) but have it in EAL as an alternative to UIO, VFIO, etc. I suppose that as DPAA2 is a kind of API, it should be possible to go this way. > > > > > > > > > > > > > >> + return 0; > > > >> +} > > > >> + > > > >> /* save real devices and drivers until the tests finishes */ > > [...] > > > > > > > > > > >> +int > > > >> +rte_eal_soc_match(struct rte_soc_driver *drv, struct rte_soc_device > > *dev) > > > >> +{ > > > >> + int i, j; > > > >> + > > > >> + RTE_VERIFY(drv != NULL && drv->id_table != NULL); > > > >> + RTE_VERIFY(dev != NULL && dev->id != NULL); > > > >> + > > > >> + for (i = 0; drv->id_table[i].compatible; ++i) { > > > >> + const char *drv_compat = drv->id_table[i].compatible; > > > >> + > > > >> + for (j = 0; dev->id[j].compatible; ++j) { > > > >> + const char *dev_compat = dev->id[j].compatible; > > > >> + > > > >> + if (!strcmp(drv_compat, dev_compat)) > > > >> + return 0; > > > >> + } > > > >> + } > > > >> + > > > >> + return 1; > > > >> +} > > > >> + > > > > A redundant empty line here... > > Yes, I will remove this. > > > > > > >> + > > > >> +static int > > > >> +rte_eal_soc_probe_one_driver(struct rte_soc_driver *drv, > > > >> + struct rte_soc_device *dev) > > > >> +{ > > > >> + int ret = 1; > > > >> + > > > > > > > > I think, the RTE_VERIFY(dev->match_fn) might be good here. > > > > It avoids any doubts about the validity of the pointer. > > > > > > That has already been done in rte_eal_soc_register which is called when > > > PMDs are registering themselves through DRIVER_REGISTER_SOC. That would > > > prevent any PMD leaking through to this stage without a proper > > > match_fn/scan_fn. > > > > Well, yes. It seems to be redundant. However, it would emphesize the fact > > that this function expects that match_fn is set. > > > > In the rte_eal_soc_register, the RTE_VERIFY says "The API requires those". > > > > But when I review I do not always see all the context. It is not safe for > > me to assume that there was probably some RTE_VERIFY in the path... It is > > not a fast path so it does not hurt the performance in anyway. > > > > You mean RTE_VERIFY should be duplicated so that code readability increases? > I am not really convinced on that. > My opinion: rte_eal_soc_*probe* series shouldn't actually worry about whether a valid PMD has been plugged in or. It should simply assume that having reached here, all is well on the PMDs definition side. > > On the contrary, placing a RTE_VERIFY here would only state the reader that it is possible to reach this code path without a valid match function. Yes, it would state that the match function must always be valid here. That's the point. > > > > > > > > > > > >> + ret = drv->match_fn(drv, dev); > > > >> + if (ret) { > > > >> + RTE_LOG(DEBUG, EAL, > > > >> + " match function failed, skipping\n"); > > > > > > > > Is this a failure? I think it is not. Failure would be if the match > > > > function cannot execute correctly. This is more like "no-match". > > > > > > The log message is misleading. This is _not_ a failure but simply a > > > 'no-match'. I will update this. > > > > > > > > > > > When debugging, I'd like to see more a message like "driver does > > not match". > > > > > > Problem would be about '' of a driver. There is already another > > > discussion about SoC capability/platform bus definitions - probably I > > > will wait for that so as to define what a '' for a driver and > > > device is. > > > In this case, the key reason for not adding such a message was because > > > it was assumed PMDs are black boxes with EAL not even assuming what > > > '' means. Anyways, it is better to discuss these things in that > > > other email. > > > > I am not sure which thread do you mean... Can you point me there, please? > > Sorry, somehow I forgot to add that: > http://dpdk.org/ml/archives/dev/2016-September/046933.html > > One of the discussion point in above thread is whether platform bus is a good assumption for a SoC driver or not. Based on that '' (or any other identifier, like 'compatible'), the log can be printed as you suggested above. Quite frankly, I don't know. The platform bus seemed the obvious starting point to me when I started working on this topic. There was no DPAA2 (as far as I know), nothing more suitable. I write custom kernel drivers as platform devices. However, if you look into PCI infrastructure you can see that the UIO, VFIO and custom approaches are available. And there is no PMD-specific scan function. I'd rather go the way it is already done to not diverge a lot from the base code. Otherwise, we can easily come to a point like "drop DPDK and write again" :). > > > > > > > > > > [...] > > > > > > > > > > > > > >> + if (drv && drv->scan_fn) { > > > >> + drv->scan_fn(); > > > >> + /* Ignore all errors from this */ > > > >> + } > > > > > > > >> + } > > > >> + > > > >> return 0; > > > >> } > > > >> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map > > b/lib/librte_eal/linuxapp/eal/rte_eal_version.map > > > >> index b9d1932..adcfe7d 100644 > > > >> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map > > > >> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map > > > >> @@ -179,5 +179,9 @@ DPDK_16.11 { > > > >> rte_eal_soc_register; > > > >> rte_eal_soc_unregister; > > > >> rte_eal_soc_dump; > > > >> + rte_eal_soc_match; > > > >> + rte_eal_soc_detach; > > > >> + rte_eal_soc_probe; > > > >> + rte_eal_soc_probe_one; > > > >> > > > >> } DPDK_16.07; > > > > > > > > Regards > > > > Jan > > > > > > > > > > I hope I have covered all your comments. That was an exhaustive review. > > > Thanks a lot for your time. > > > > > > Lets work to resolve the architectural issues revolving around SoC > > > scan/match. > > > > ;) > > Phew! > I am already loosing track of things you have suggested. > What if we can chat on IRC and resolve the issue surrounding the 'default match/scan', 'platform or not platform dependent SoC' etc? I think it would save a lot of ping-pong effort. > (For today (20/Sep), I might not be available, but I am OK any other time). Sure... write me when you are available. I maybe temporarily away. > > > > > > > > > - > > > Shreyansh > > > > > > > > -- > > Jan Viktorin E-mail: Viktorin@RehiveTech.com > > System Architect Web: www.RehiveTech.com > > RehiveTech > > Brno, Czech Republic > > Thanks a lot. > > - > Shreyansh -- Jan Viktorin E-mail: Viktorin@RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic