On Tue, 20 Sep 2016 06:46:31 +0000
Shreyansh Jain [off-list ref] wrote:

Hi Jan,

quoted

-----Original Message-----
From: Jan Viktorin [mailto:viktorin@rehivetech.com]
Sent: Monday, September 19, 2016 5:04 PM
To: Shreyansh Jain <redacted>
Cc: dev@dpdk.org; Hemant Agrawal <redacted>
Subject: Re: [PATCH v3 06/15] eal/soc: implement probing of drivers

On Mon, 19 Sep 2016 12:17:53 +0530
Shreyansh Jain [off-list ref] wrote:
  

quoted

Hi Jan,

[...]

quoted

quoted

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 ;).

quoted

quoted

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).
  

quoted

implement its own match/scan and these would be called from EAL as  

expected.  

quoted

quoted

quoted

+{
+	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  

quoted

quoted

quoted

+	 * that drivers are indeed registered
+	*/
+
+	/* rte_eal_soc_init is called by rte_eal_init, which in turn  

calls the  

quoted

quoted

quoted

+	 * 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.

quoted

quoted

quoted

quoted

+	 */
+	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.
 

quoted

quoted

+
+	/* 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.

quoted

quoted

quoted

quoted

+	return 0;
+}
+
 /* save real devices and drivers until the tests finishes */  

[...]

quoted

quoted

quoted

quoted

+int
+rte_eal_soc_match(struct rte_soc_driver *drv, struct rte_soc_device  

*dev)  

quoted

quoted

quoted

+{
+	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. 

quoted

quoted

quoted

quoted

+
+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.

quoted

quoted

quoted

quoted

+	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.
 

quoted

When debugging, I'd like to see more a message like "driver <name> does  

not match".  

quoted

Problem would be about '<name>' 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 '<name>' 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
'<name>' 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 '<name>' (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" :).

quoted

quoted

quoted

[...]

quoted

quoted

quoted

quoted

+		if (drv && drv->scan_fn) {
+			drv->scan_fn();
+			/* Ignore all errors from this */
+		}  

quoted

+	}
+
 	return 0;
 }

diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map  

b/lib/librte_eal/linuxapp/eal/rte_eal_version.map  

quoted

quoted

quoted

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.

quoted

quoted

-
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