Re: [dpdk-dev] [PATCH 01/13] eal/log: introduce log register macro

[David Marchand <david.marchand@redhat.com>]

On Fri, Jun 26, 2020 at 2:38 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > Or no declaration in macro and leave code as it is.
>
> I dont see anything wrong with that. Do you have technical rationale
> for the above rule?

Avoid global symbols when unneeded.
Global symbols can be hidden with shared build, but that's not the
case with static build.

This is why we try to maintain a namespace for DPDK api.

For logtypes, a lot of those are already global and nobody complained
about them (lucky ?).

With this patch, the static ones, that we would convert to global
symbols, are the following:
drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c:static int
fpga_5gnr_fec_logtype;
drivers/baseband/fpga_lte_fec/fpga_lte_fec.c:static int fpga_lte_fec_logtype;
drivers/baseband/null/bbdev_null.c:static int bbdev_null_logtype;
drivers/baseband/turbo_sw/bbdev_turbo_software.c:static int
bbdev_turbo_sw_logtype;
drivers/net/af_packet/rte_eth_af_packet.c:static int af_packet_logtype;
drivers/net/af_xdp/rte_eth_af_xdp.c:static int af_xdp_logtype;
drivers/net/kni/rte_eth_kni.c:static int eth_kni_logtype;
drivers/net/null/rte_eth_null.c:static int eth_null_logtype;
drivers/net/pcap/rte_eth_pcap.c:static int eth_pcap_logtype;
drivers/net/ring/rte_eth_ring.c:static int eth_ring_logtype;
drivers/net/softnic/rte_eth_softnic.c:static int pmd_softnic_logtype;
drivers/net/vdev_netvsc/vdev_netvsc.c:static int vdev_netvsc_logtype;
drivers/net/vhost/rte_eth_vhost.c:static int vhost_logtype;
drivers/vdpa/ifc/ifcvf_vdpa.c:static int ifcvf_vdpa_logtype;
lib/librte_bbdev/rte_bbdev.c:static int bbdev_logtype;
lib/librte_cfgfile/rte_cfgfile.c:static int cfgfile_logtype;
lib/librte_eventdev/rte_event_timer_adapter.c:static int evtim_logtype;
lib/librte_eventdev/rte_event_timer_adapter.c:static int evtim_buffer_logtype;
lib/librte_eventdev/rte_event_timer_adapter.c:static int evtim_svc_logtype;
lib/librte_pdump/rte_pdump.c:static int pdump_logtype;

All of them follow some kind of convention of finishing with _logtype.
Is this enough and we won't trigger link issues in existing applications?
Probably not possible to tell.

There was a similar discussion with Gaetan in the pci bus code, and so
far we are still in this status quo for global symbols.

So ok, let's go with this embedded global symbol.
If we hit an issue with static linking, we will need a tree-wide cleanup.


> > > > - Having components set log levels at init time in the macro is a bug to me.
> > > > This has been worked around with
> > > > rte_log_register/rte_log_register_and_pick_level but the initial
> > > > problem is that rte_log_set_level* should only be called by the user.
> > >
> > > I agree with the below stuff, That's is not introduced by this patch.
> > > It is already there.
> > > Be it macro or no macro code.
> > >
> > > I think this patch helps to change to new scheme as it takes care of
> > > changing the
> > > registration part to commonplace so that we can set to the same level
> > > in the future if
> > > everyone agrees to it
> >
> > We will still expose this macro meaning that we will have an API breakage later.
> > So if we go with introducing this, let's make it good from the start.
>
> But Is everyone OK with removing the "level" from the register before
> we think about
> breaking the API later?. I see PMD uses multiple levels in the same
> PMD for different
> purposes.

It is an API fix to me, as there should be no place for "level" interpretation.


Just a note, a v2 would be necessary for the
rte_log_register_type_and_pick_level use in any case.

-- 
David Marchand