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

["Guo, Junfeng" <junfeng.guo@intel.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
> Sent: Wednesday, September 7, 2022 19:17
> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>;
> awogbemila@google.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Subject: Re: [PATCH v2 02/10] net/gve: add logs and OS specific
> implementation
> 
> 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@xilinx.com>
> >> Sent: Friday, September 2, 2022 01:21
> >> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> >> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>;
> >> awogbemila@google.com; Richardson, Bruce
> >> <bruce.richardson@intel.com>; Wang, Haiyue
> <haiyue.wang@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@intel.com>
> >>> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> >>> Signed-off-by: Junfeng Guo <junfeng.guo@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.

Yes, already did as this form in the coming version gve pmd code. Thanks!

> 
> >>
> >>
> >> 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?

Well, these two types may be used to show init/driver logs, respectively.
But It seems that there is no such specific need to use two log types in the
GVE PMD. Anyway, I think it is good time to keep the code clean and not 
just inherit from previous drivers. We can add new log type in the future
if it's required. Thanks!