[dpdk-dev,v2,11/18] devargs: simplify implementation

  Re-use existing code, remove incorrect comments.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_devargs.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
  

Hello Gaetan,

On Thursday 12 October 2017 01:51 PM, Gaetan Rivet wrote:
> Re-use existing code, remove incorrect comments.
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>   lib/librte_eal/common/eal_common_devargs.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 49cc3b8..1d87cd9 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -153,21 +153,19 @@ rte_eal_devargs_insert(struct rte_devargs *da)
>   	return 0;
>   }
>   

While trying to work on this patch, I noticed that the complete series 
(including "Move PCI away from EAL") is not cleanly applicable on 
current master (17.11 RC1). I thought it would be some tiny issues.

But there are some issues which I couldn't pass, Like...

> -/* store a whitelist parameter for later parsing */
>   int

In the this function

> -rte_eal_devargs_add(const char *devargs_str)
> +rte_eal_devargs_add(const char *dev)
>   {
>   	struct rte_devargs *devargs = NULL;
> -	const char *dev = devargs_str;
>   
> -	/* use calloc instead of rte_zmalloc as it's called early at init */
>   	devargs = calloc(1, sizeof(*devargs));
>   	if (devargs == NULL)
>   		goto fail;
>   
>   	if (rte_eal_devargs_parse(devargs, "%s", dev))
>   		goto fail;

These lines don't exist in your patch

---
59c2ba6c 172)   if (bus->conf.probe_mode == RTE_BUS_PROBE_UNDEFINED) {
b631f3b0 173)           if (devargs->policy == RTE_DEV_WHITELISTED)
59c2ba6c 174)                   bus->conf.probe_mode = 
RTE_BUS_PROBE_WHITELIST;
b631f3b0 175)           else if (devargs->policy == RTE_DEV_BLACKLISTED)
59c2ba6c 176)                   bus->conf.probe_mode = 
RTE_BUS_PROBE_BLACKLIST;
02823c1d 177)   }
bf6dea0e 178)   TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
bf6dea0e 179)   return 0;
0215a4c6 180)
---
(Some introduced by the move PCI series, but others like b631f3b0 are 
very old ~17.08)


> -	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
> +	if (rte_eal_devargs_insert(devargs))
> +		goto fail;

And hence, I don't know whether you intend to insert the above line 
after or before checking PROBE.

>   	return 0;
>   
>   fail:
>

Maybe I am doing something wrong here - any ideas? Can you send an 
updated/rebased version on current master HEAD?

-
Shreyansh
  

Hello Gaetan,

Please ignore this email (reason inline)

On Monday 16 October 2017 05:09 PM, Shreyansh Jain wrote:
> Hello Gaetan,
> 
> On Thursday 12 October 2017 01:51 PM, Gaetan Rivet wrote:
>> Re-use existing code, remove incorrect comments.
>>
>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>> ---
>>   lib/librte_eal/common/eal_common_devargs.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_devargs.c 
>> b/lib/librte_eal/common/eal_common_devargs.c
>> index 49cc3b8..1d87cd9 100644
>> --- a/lib/librte_eal/common/eal_common_devargs.c
>> +++ b/lib/librte_eal/common/eal_common_devargs.c
>> @@ -153,21 +153,19 @@ rte_eal_devargs_insert(struct rte_devargs *da)
>>       return 0;
>>   }
> 
> While trying to work on this patch, I noticed that the complete series 
> (including "Move PCI away from EAL") is not cleanly applicable on 
> current master (17.11 RC1). I thought it would be some tiny issues.
> 
> But there are some issues which I couldn't pass, Like...
> 
>> -/* store a whitelist parameter for later parsing */
>>   int
> 
> In the this function
> 
>> -rte_eal_devargs_add(const char *devargs_str)
>> +rte_eal_devargs_add(const char *dev)
>>   {
>>       struct rte_devargs *devargs = NULL;
>> -    const char *dev = devargs_str;
>> -    /* use calloc instead of rte_zmalloc as it's called early at init */
>>       devargs = calloc(1, sizeof(*devargs));
>>       if (devargs == NULL)
>>           goto fail;
>>       if (rte_eal_devargs_parse(devargs, "%s", dev))
>>           goto fail;
> 
> These lines don't exist in your patch
> 
> ---
> 59c2ba6c 172)   if (bus->conf.probe_mode == RTE_BUS_PROBE_UNDEFINED) {
> b631f3b0 173)           if (devargs->policy == RTE_DEV_WHITELISTED)
> 59c2ba6c 174)                   bus->conf.probe_mode = 
> RTE_BUS_PROBE_WHITELIST;
> b631f3b0 175)           else if (devargs->policy == RTE_DEV_BLACKLISTED)
> 59c2ba6c 176)                   bus->conf.probe_mode = 
> RTE_BUS_PROBE_BLACKLIST;
> 02823c1d 177)   }
> bf6dea0e 178)   TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
> bf6dea0e 179)   return 0;
> 0215a4c6 180)
> ---
> (Some introduced by the move PCI series, but others like b631f3b0 are 
> very old ~17.08)
> 
> 
>> -    TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
>> +    if (rte_eal_devargs_insert(devargs))
>> +        goto fail;
> 
> And hence, I don't know whether you intend to insert the above line 
> after or before checking PROBE.
> 
>>       return 0;
>>   fail:
>>
> 
> Maybe I am doing something wrong here - any ideas? Can you send an 
> updated/rebased version on current master HEAD?

Just after sending this, I noticed that I had not applied the "Bus 
control framework" patch set which the "devargs..." cover letter talks 
about.
I will try with that and confirm if there is still any issue.

> 
> -
> Shreyansh
>
  

On Mon, Oct 16, 2017 at 05:12:37PM +0530, Shreyansh Jain wrote:
> Hello Gaetan,
> 
> Please ignore this email (reason inline)
> 

Hello Shreyansh,

Thanks for reading this patchset, and sorry about the confusion (the
previous cover-letter only listed the PCI bus move, the bus control was
only added in this version).

I'd be very happy to see these series integrated for this release or the
next, and as such your opinion would matter a great deal.

However, I have found important issues that I am still working on with
the PCI bus move. I am trying to fix this in time, but I'm not sure yet
to succeed soon enough.

It would block the rest. I could still redo both series without the PCI
bus move but the bus control scheme is necessary for the devargs
cleanup.

I don't know if this bus control scheme is interesting enough though.
I need additional opinions about it.

Anyway, thanks,
  

Hello Gaetan,

On Monday 16 October 2017 07:12 PM, Gaëtan Rivet wrote:
> On Mon, Oct 16, 2017 at 05:12:37PM +0530, Shreyansh Jain wrote:
>> Hello Gaetan,
>>
>> Please ignore this email (reason inline)
>>
> 
> Hello Shreyansh,
> 
> Thanks for reading this patchset, and sorry about the confusion (the
> previous cover-letter only listed the PCI bus move, the bus control was
> only added in this version).

That's OK. I mixed up the cover-letters anyway.
Probably the complete series needs a rebase as some patches have changes 
exactly same locations in EAL where this patch impacts (at least for PCI 
movement patches, I found patches had fuzz factor)

> 
> I'd be very happy to see these series integrated for this release or the
> next, and as such your opinion would matter a great deal.

I do understand the importance of this patch series - but I have not 
been able to devote enough time to this lately. Even now, I am not sure 
if I would be able to completely review all within 17.11 timeframe as it 
is quite big. (And I have some personal time-off coming up soon!)

> 
> However, I have found important issues that I am still working on with
> the PCI bus move. I am trying to fix this in time, but I'm not sure yet
> to succeed soon enough.

I understand.
It is a complex series - specially the build break in PCI movement on 
per patch basis - that would be difficult to solve. Probably that 
requires adding dummy functions/variables to allow compilation and then 
moving them out. It is indeed tough choice.

> 
> It would block the rest. I could still redo both series without the PCI
> bus move but the bus control scheme is necessary for the devargs
> cleanup.

Probably it is best to postpone for 18.02. It indeed is difficult to let 
a proposal fall through a planned release - but this work is really 
critical as it impacts a lot of people (PCI, Buses, args etc).
Just my personal opinion.

> 
> I don't know if this bus control scheme is interesting enough though.
> I need additional opinions about it.

I haven't started reading the Bus control yet - I am still focusing on 
PCI movement and then I had planned the devargs. But, I will start 
looking into this as well. I would try to add whatever opinion I can.

> 
> Anyway, thanks,
> 

Always welcome! But, that's what the community is for. :D

-
Shreyansh