diff mbox series

[5/5] sfc: don't use RTE_LOGTYPE_PMD

Message ID 20190226213424.10567-6-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series no longer use RTE_LOGTYPE_PMD |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger Feb. 26, 2019, 9:34 p.m. UTC 
  The sfc driver was still using RTE_LOGTYPE_PMD which was superseded
by local logging.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/sfc/sfc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit Feb. 27, 2019, 11:21 a.m. UTC | #1 
On 2/26/2019 9:34 PM, Stephen Hemminger wrote:
> The sfc driver was still using RTE_LOGTYPE_PMD which was superseded
> by local logging.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/sfc/sfc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/sfc/sfc.c b/drivers/net/sfc/sfc.c
> index 898603884fa0..2cd7126015fd 100644
> --- a/drivers/net/sfc/sfc.c
> +++ b/drivers/net/sfc/sfc.c
> @@ -1115,12 +1115,12 @@ sfc_register_logtype(const struct rte_pci_addr *pci_addr,
>  		++lt_prefix_str_size; /* Reserve space for prefix separator */
>  		lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
>  	} else {
> -		return RTE_LOGTYPE_PMD;
> +		return sfc_logtype_driver;
>  	}
>  
>  	lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
>  	if (lt_str == NULL)
> -		return RTE_LOGTYPE_PMD;
> +		return sfc_logtype_driver;
>  
>  	strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
>  	lt_str[lt_prefix_str_size - 1] = '.';
> 

Overall I think it is good idea to remove RTE_LOGTYPE_PMD, but sfc has a few
more usage of it around same manner, as a fallback value if allocating dynamic
one fails.


Andrew,

Can be possible to update this sfc patch to completely eliminate RTE_LOGTYPE_PMD
usage? What do you think?

Thanks,
ferruh
Andrew Rybchenko Feb. 27, 2019, 11:24 a.m. UTC | #2 
On 2/27/19 2:21 PM, Ferruh Yigit wrote:
> On 2/26/2019 9:34 PM, Stephen Hemminger wrote:
>> The sfc driver was still using RTE_LOGTYPE_PMD which was superseded
>> by local logging.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
>>   drivers/net/sfc/sfc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/sfc/sfc.c b/drivers/net/sfc/sfc.c
>> index 898603884fa0..2cd7126015fd 100644
>> --- a/drivers/net/sfc/sfc.c
>> +++ b/drivers/net/sfc/sfc.c
>> @@ -1115,12 +1115,12 @@ sfc_register_logtype(const struct rte_pci_addr *pci_addr,
>>   		++lt_prefix_str_size; /* Reserve space for prefix separator */
>>   		lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
>>   	} else {
>> -		return RTE_LOGTYPE_PMD;
>> +		return sfc_logtype_driver;
>>   	}
>>   
>>   	lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
>>   	if (lt_str == NULL)
>> -		return RTE_LOGTYPE_PMD;
>> +		return sfc_logtype_driver;
>>   
>>   	strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
>>   	lt_str[lt_prefix_str_size - 1] = '.';
>>
> Overall I think it is good idea to remove RTE_LOGTYPE_PMD, but sfc has a few
> more usage of it around same manner, as a fallback value if allocating dynamic
> one fails.
>
>
> Andrew,
>
> Can be possible to update this sfc patch to completely eliminate RTE_LOGTYPE_PMD
> usage? What do you think?
>
> Thanks,
> ferruh

I'm OK to use  sfc_logtype_driverif dynamic log type register fails, but
what should I do if sfc_logtype_driverregister fails?

Andrew.
Ferruh Yigit Feb. 27, 2019, 11:50 a.m. UTC | #3 
On 2/27/2019 11:24 AM, Andrew Rybchenko wrote:
> On 2/27/19 2:21 PM, Ferruh Yigit wrote:
>> On 2/26/2019 9:34 PM, Stephen Hemminger wrote:
>>> The sfc driver was still using RTE_LOGTYPE_PMD which was superseded
>>> by local logging.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>>   drivers/net/sfc/sfc.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/sfc/sfc.c b/drivers/net/sfc/sfc.c
>>> index 898603884fa0..2cd7126015fd 100644
>>> --- a/drivers/net/sfc/sfc.c
>>> +++ b/drivers/net/sfc/sfc.c
>>> @@ -1115,12 +1115,12 @@ sfc_register_logtype(const struct rte_pci_addr *pci_addr,
>>>   		++lt_prefix_str_size; /* Reserve space for prefix separator */
>>>   		lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
>>>   	} else {
>>> -		return RTE_LOGTYPE_PMD;
>>> +		return sfc_logtype_driver;
>>>   	}
>>>   
>>>   	lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
>>>   	if (lt_str == NULL)
>>> -		return RTE_LOGTYPE_PMD;
>>> +		return sfc_logtype_driver;
>>>   
>>>   	strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
>>>   	lt_str[lt_prefix_str_size - 1] = '.';
>>>
>> Overall I think it is good idea to remove RTE_LOGTYPE_PMD, but sfc has a few
>> more usage of it around same manner, as a fallback value if allocating dynamic
>> one fails.
>>
>>
>> Andrew,
>>
>> Can be possible to update this sfc patch to completely eliminate RTE_LOGTYPE_PMD
>> usage? What do you think?
>>
>> Thanks,
>> ferruh
> 
> I'm OK to use  sfc_logtype_driverif dynamic log type register fails, but
> what should I do if sfc_logtype_driverregister fails?

Right. Same concern is valid for all dynamic log registration but we are
ignoring it.

What do you think instead of each pmd/lib cover the error case,
'rte_log_register_type_and_pick_level()' & 'rte_log_register()' always return
valid value. Like if they fail internally, return 'RTE_LOGTYPE_GENERIC' kind of
value.
Ferruh Yigit Feb. 27, 2019, 12:19 p.m. UTC | #4 
On 2/27/2019 11:50 AM, Ferruh Yigit wrote:
> On 2/27/2019 11:24 AM, Andrew Rybchenko wrote:
>> On 2/27/19 2:21 PM, Ferruh Yigit wrote:
>>> On 2/26/2019 9:34 PM, Stephen Hemminger wrote:
>>>> The sfc driver was still using RTE_LOGTYPE_PMD which was superseded
>>>> by local logging.
>>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>> ---
>>>>   drivers/net/sfc/sfc.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/sfc/sfc.c b/drivers/net/sfc/sfc.c
>>>> index 898603884fa0..2cd7126015fd 100644
>>>> --- a/drivers/net/sfc/sfc.c
>>>> +++ b/drivers/net/sfc/sfc.c
>>>> @@ -1115,12 +1115,12 @@ sfc_register_logtype(const struct rte_pci_addr *pci_addr,
>>>>   		++lt_prefix_str_size; /* Reserve space for prefix separator */
>>>>   		lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
>>>>   	} else {
>>>> -		return RTE_LOGTYPE_PMD;
>>>> +		return sfc_logtype_driver;
>>>>   	}
>>>>   
>>>>   	lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
>>>>   	if (lt_str == NULL)
>>>> -		return RTE_LOGTYPE_PMD;
>>>> +		return sfc_logtype_driver;
>>>>   
>>>>   	strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
>>>>   	lt_str[lt_prefix_str_size - 1] = '.';
>>>>
>>> Overall I think it is good idea to remove RTE_LOGTYPE_PMD, but sfc has a few
>>> more usage of it around same manner, as a fallback value if allocating dynamic
>>> one fails.
>>>
>>>
>>> Andrew,
>>>
>>> Can be possible to update this sfc patch to completely eliminate RTE_LOGTYPE_PMD
>>> usage? What do you think?
>>>
>>> Thanks,
>>> ferruh
>>
>> I'm OK to use  sfc_logtype_driverif dynamic log type register fails, but
>> what should I do if sfc_logtype_driverregister fails?
> 
> Right. Same concern is valid for all dynamic log registration but we are
> ignoring it.
> 
> What do you think instead of each pmd/lib cover the error case,
> 'rte_log_register_type_and_pick_level()' & 'rte_log_register()' always return
> valid value. Like if they fail internally, return 'RTE_LOGTYPE_GENERIC' kind of
> value.

Changing the log functions is one above step, are you OK to replace all
occurrence of 'RTE_LOGTYPE_PMD' with 'sfc_logtype_driver' in
'sfc_register_logtype()', there is one more occurrence than this patch updates.

I see your point to keep the one in 'sfc_driver_register_logtype()', perhaps we
can address it later separately with above suggestion or something else.
Andrew Rybchenko Feb. 27, 2019, 1:05 p.m. UTC | #5 
On 2/27/19 3:19 PM, Ferruh Yigit wrote:
> On 2/27/2019 11:50 AM, Ferruh Yigit wrote:
>> On 2/27/2019 11:24 AM, Andrew Rybchenko wrote:
>>> On 2/27/19 2:21 PM, Ferruh Yigit wrote:
>>>> On 2/26/2019 9:34 PM, Stephen Hemminger wrote:
>>>>> The sfc driver was still using RTE_LOGTYPE_PMD which was superseded
>>>>> by local logging.
>>>>>
>>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>>> ---
>>>>>    drivers/net/sfc/sfc.c | 4 ++--
>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/sfc/sfc.c b/drivers/net/sfc/sfc.c
>>>>> index 898603884fa0..2cd7126015fd 100644
>>>>> --- a/drivers/net/sfc/sfc.c
>>>>> +++ b/drivers/net/sfc/sfc.c
>>>>> @@ -1115,12 +1115,12 @@ sfc_register_logtype(const struct rte_pci_addr *pci_addr,
>>>>>    		++lt_prefix_str_size; /* Reserve space for prefix separator */
>>>>>    		lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
>>>>>    	} else {
>>>>> -		return RTE_LOGTYPE_PMD;
>>>>> +		return sfc_logtype_driver;
>>>>>    	}
>>>>>    
>>>>>    	lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
>>>>>    	if (lt_str == NULL)
>>>>> -		return RTE_LOGTYPE_PMD;
>>>>> +		return sfc_logtype_driver;
>>>>>    
>>>>>    	strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
>>>>>    	lt_str[lt_prefix_str_size - 1] = '.';
>>>>>
>>>> Overall I think it is good idea to remove RTE_LOGTYPE_PMD, but sfc has a few
>>>> more usage of it around same manner, as a fallback value if allocating dynamic
>>>> one fails.
>>>>
>>>>
>>>> Andrew,
>>>>
>>>> Can be possible to update this sfc patch to completely eliminate RTE_LOGTYPE_PMD
>>>> usage? What do you think?
>>>>
>>>> Thanks,
>>>> ferruh
>>> I'm OK to use  sfc_logtype_driverif dynamic log type register fails, but
>>> what should I do if sfc_logtype_driverregister fails?
>> Right. Same concern is valid for all dynamic log registration but we are
>> ignoring it.
>>
>> What do you think instead of each pmd/lib cover the error case,
>> 'rte_log_register_type_and_pick_level()' & 'rte_log_register()' always return
>> valid value. Like if they fail internally, return 'RTE_LOGTYPE_GENERIC' kind of
>> value.

Above makes sense.

> Changing the log functions is one above step, are you OK to replace all
> occurrence of 'RTE_LOGTYPE_PMD' with 'sfc_logtype_driver' in
> 'sfc_register_logtype()', there is one more occurrence than this patch updates.

Yes

> I see your point to keep the one in 'sfc_driver_register_logtype()', perhaps we
> can address it later separately with above suggestion or something else.

OK.

Thanks,
Andrew.
Stephen Hemminger Feb. 27, 2019, 6:30 p.m. UTC | #6 
On Wed, 27 Feb 2019 14:24:21 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 2/27/19 2:21 PM, Ferruh Yigit wrote:
> > On 2/26/2019 9:34 PM, Stephen Hemminger wrote:  
> >> The sfc driver was still using RTE_LOGTYPE_PMD which was superseded
> >> by local logging.
> >>
> >> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >> ---
> >>   drivers/net/sfc/sfc.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/sfc/sfc.c b/drivers/net/sfc/sfc.c
> >> index 898603884fa0..2cd7126015fd 100644
> >> --- a/drivers/net/sfc/sfc.c
> >> +++ b/drivers/net/sfc/sfc.c
> >> @@ -1115,12 +1115,12 @@ sfc_register_logtype(const struct rte_pci_addr *pci_addr,
> >>   		++lt_prefix_str_size; /* Reserve space for prefix separator */
> >>   		lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
> >>   	} else {
> >> -		return RTE_LOGTYPE_PMD;
> >> +		return sfc_logtype_driver;
> >>   	}
> >>   
> >>   	lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
> >>   	if (lt_str == NULL)
> >> -		return RTE_LOGTYPE_PMD;
> >> +		return sfc_logtype_driver;
> >>   
> >>   	strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
> >>   	lt_str[lt_prefix_str_size - 1] = '.';
> >>  
> > Overall I think it is good idea to remove RTE_LOGTYPE_PMD, but sfc has a few
> > more usage of it around same manner, as a fallback value if allocating dynamic
> > one fails.
> >
> >
> > Andrew,
> >
> > Can be possible to update this sfc patch to completely eliminate RTE_LOGTYPE_PMD
> > usage? What do you think?
> >
> > Thanks,
> > ferruh  
> 
> I'm OK to use  sfc_logtype_driverif dynamic log type register fails, but
> what should I do if sfc_logtype_driverregister fails?
> 
> Andrew.

I don't like drivers that try to do something different than every other driver in DPDK.
The solarflare driver is doing lots of extra effort to have multiple fine grain log types.
Not sure if there is any value to this. In a real world situation for diagnosis you would
want to turn on logs across everything in the driver, then filter as needed later.
Andrew Rybchenko Feb. 28, 2019, 6:49 a.m. UTC | #7 
On 2/27/19 9:30 PM, Stephen Hemminger wrote:
> On Wed, 27 Feb 2019 14:24:21 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> On 2/27/19 2:21 PM, Ferruh Yigit wrote:
>>> On 2/26/2019 9:34 PM, Stephen Hemminger wrote:
>>>> The sfc driver was still using RTE_LOGTYPE_PMD which was superseded
>>>> by local logging.
>>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>> ---
>>>>    drivers/net/sfc/sfc.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/sfc/sfc.c b/drivers/net/sfc/sfc.c
>>>> index 898603884fa0..2cd7126015fd 100644
>>>> --- a/drivers/net/sfc/sfc.c
>>>> +++ b/drivers/net/sfc/sfc.c
>>>> @@ -1115,12 +1115,12 @@ sfc_register_logtype(const struct rte_pci_addr *pci_addr,
>>>>    		++lt_prefix_str_size; /* Reserve space for prefix separator */
>>>>    		lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
>>>>    	} else {
>>>> -		return RTE_LOGTYPE_PMD;
>>>> +		return sfc_logtype_driver;
>>>>    	}
>>>>    
>>>>    	lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
>>>>    	if (lt_str == NULL)
>>>> -		return RTE_LOGTYPE_PMD;
>>>> +		return sfc_logtype_driver;
>>>>    
>>>>    	strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
>>>>    	lt_str[lt_prefix_str_size - 1] = '.';
>>>>   
>>> Overall I think it is good idea to remove RTE_LOGTYPE_PMD, but sfc has a few
>>> more usage of it around same manner, as a fallback value if allocating dynamic
>>> one fails.
>>>
>>>
>>> Andrew,
>>>
>>> Can be possible to update this sfc patch to completely eliminate RTE_LOGTYPE_PMD
>>> usage? What do you think?
>>>
>>> Thanks,
>>> ferruh
>> I'm OK to use  sfc_logtype_driverif dynamic log type register fails, but
>> what should I do if sfc_logtype_driverregister fails?
>>
>> Andrew.
> I don't like drivers that try to do something different than every other driver in DPDK.
> The solarflare driver is doing lots of extra effort to have multiple fine grain log types.
> Not sure if there is any value to this. In a real world situation for diagnosis you would
> want to turn on logs across everything in the driver, then filter as needed later.

We really use logging configuration facilities which we have in the 
driver and,
thanks to dynamic logging design (IMO really good design), both your and
our logging usage scenarios can coexist.

Andrew.
diff mbox series

Patch

diff --git a/drivers/net/sfc/sfc.c b/drivers/net/sfc/sfc.c
index 898603884fa0..2cd7126015fd 100644
--- a/drivers/net/sfc/sfc.c
+++ b/drivers/net/sfc/sfc.c
@@ -1115,12 +1115,12 @@  sfc_register_logtype(const struct rte_pci_addr *pci_addr,
 		++lt_prefix_str_size; /* Reserve space for prefix separator */
 		lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
 	} else {
-		return RTE_LOGTYPE_PMD;
+		return sfc_logtype_driver;
 	}
 
 	lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
 	if (lt_str == NULL)
-		return RTE_LOGTYPE_PMD;
+		return sfc_logtype_driver;
 
 	strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
 	lt_str[lt_prefix_str_size - 1] = '.';