[PATCH v2 02/10] net/gve: add logs and OS specific implementation

[Ferruh Yigit]

On 9/7/2022 7:58 AM, Guo, Junfeng wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit at xilinx.com>
>> Sent: Friday, September 2, 2022 01:21
>> To: Guo, Junfeng <junfeng.guo at intel.com>; Zhang, Qi Z
>> <qi.z.zhang at intel.com>; Wu, Jingjing <jingjing.wu at intel.com>
>> Cc: dev at dpdk.org; Li, Xiaoyun <xiaoyun.li at intel.com>;
>> awogbemila at google.com; Richardson, Bruce
>> <bruce.richardson at intel.com>; Wang, Haiyue <haiyue.wang at intel.com>
>> Subject: Re: [PATCH v2 02/10] net/gve: add logs and OS specific
>> implementation
>>
>> On 8/29/2022 9:41 AM, Junfeng Guo wrote:
>>
>>>
>>> Add GVE PMD logs.
>>> Add some MACRO definitions and memory operations which are specific
>>> for DPDK.
>>>
>>> Signed-off-by: Haiyue Wang <haiyue.wang at intel.com>
>>> Signed-off-by: Xiaoyun Li <xiaoyun.li at intel.com>
>>> Signed-off-by: Junfeng Guo <junfeng.guo at intel.com>
>>
>> <...>
>>
>>> diff --git a/drivers/net/gve/gve_logs.h b/drivers/net/gve/gve_logs.h
>>> new file mode 100644
>>> index 0000000000..a050253f59
>>> --- /dev/null
>>> +++ b/drivers/net/gve/gve_logs.h
>>> @@ -0,0 +1,22 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(C) 2022 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _GVE_LOGS_H_
>>> +#define _GVE_LOGS_H_
>>> +
>>> +extern int gve_logtype_init;
>>> +extern int gve_logtype_driver;
>>> +
>>> +#define PMD_INIT_LOG(level, fmt, args...) \
>>> +       rte_log(RTE_LOG_ ## level, gve_logtype_init, "%s(): " fmt "\n", \
>>> +               __func__, ##args)
>>> +
>>> +#define PMD_DRV_LOG_RAW(level, fmt, args...) \
>>> +       rte_log(RTE_LOG_ ## level, gve_logtype_driver, "%s(): " fmt, \
>>> +               __func__, ## args)
>>> + > +#define PMD_DRV_LOG(level, fmt, args...) \
>>> +       PMD_DRV_LOG_RAW(level, fmt "\n", ## args)
>>> +
>>
>> Why 'PMD_DRV_LOG_RAW' is needed, why not directly use
>> 'PMD_DRV_LOG'?
> 
> It seems that the _RAW macro was first introduced at i40e driver logs file.
> Since sometimes the trailing '\n' is added at the end of the log message in
> the base code, the PMD_DRV_LOG_RAW macro that will not add one is
> used to keep consistent of the new line character.
> 
> Well, looks that the macro PMD_DRV_LOG_RAW is somewhat redundant.
> I think it's ok to remove PMD_DRV_LOG_RAW and keep all the log messages
> end without the trailing '\n'. Thanks!
> 

Or you can add '\n' to 'PMD_DRV_LOG', to not change all logs. Only 
having two macro seems unnecessary.

>>
>>
>> Do you really need two different log types? How do you differentiate
>> 'init' & 'driver' types? As far as I can see there is mixed usage of them.
> 
> The PMD_INIT_LOG is used at the init stage, while the PMD_DRV_LOG
> is used at the driver normal running stage. I agree that there might be
> mixed usage of these two macros. I'll try to check all these usages and
> update them at correct conditions in the coming versions.
> If you insist that only one log type is needed to keep the code clean,
> then I could update them as you expected. Thanks!
> 

I do not insist, but it looks like you are complicating things, is there 
really a benefit to have two different log types?