Re: [dpdk-dev] [PATCH v7 1/6] Move common functions in eal_thread.c

[Neil Horman <nhorman@tuxdriver.com>]

On Sat, Apr 25, 2015 at 05:09:01PM -0700, Ravi Kerur wrote:
> On Sat, Apr 25, 2015 at 6:02 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Sat, Apr 25, 2015 at 08:32:42AM -0400, Neil Horman wrote:
> > > On Fri, Apr 24, 2015 at 06:45:06PM -0700, Ravi Kerur wrote:
> > > > On Fri, Apr 24, 2015 at 2:24 PM, Ravi Kerur <rkerur@gmail.com> wrote:
> > > >
> > > > >
> > > > >
> > > > > On Fri, Apr 24, 2015 at 12:51 PM, Neil Horman <nhorman@tuxdriver.com
> > >
> > > > > wrote:
> > > > >
> > > > >> On Fri, Apr 24, 2015 at 12:21:23PM -0700, Ravi Kerur wrote:
> > > > >> > On Fri, Apr 24, 2015 at 11:53 AM, Neil Horman <
> > nhorman@tuxdriver.com>
> > > > >> wrote:
> > > > >> >
> > > > >> > > On Fri, Apr 24, 2015 at 09:45:24AM -0700, Ravi Kerur wrote:
> > > > >> > > > On Fri, Apr 24, 2015 at 8:22 AM, Neil Horman <
> > nhorman@tuxdriver.com
> > > > >> >
> > > > >> > > wrote:
> > > > >> > > >
> > > > >> > > > > On Fri, Apr 24, 2015 at 08:14:04AM -0700, Ravi Kerur wrote:
> > > > >> > > > > > On Fri, Apr 24, 2015 at 6:51 AM, Neil Horman <
> > > > >> nhorman@tuxdriver.com>
> > > > >> > > > > wrote:
> > > > >> > > > > >
> > > > >> > > > > > > On Thu, Apr 23, 2015 at 02:35:31PM -0700, Ravi Kerur
> > wrote:
> > > > >> > > > > > > > Changes in v7
> > > > >> > > > > > > > Remove _setname_ pthread calls.
> > > > >> > > > > > > > Use rte_gettid() API in RTE_LOG to print thread_id.
> > > > >> > > > > > > >
> > > > >> > > > > > > > Changes in v6
> > > > >> > > > > > > > Remove RTE_EXEC_ENV_BSDAPP from eal_common_thread.c
> > file.
> > > > >> > > > > > > > Add pthread_setname_np/pthread_set_name_np for
> > Linux/FreeBSD
> > > > >> > > > > > > > respectively. Plan to use _getname_ in RTE_LOG when
> > > > >> available.
> > > > >> > > > > > > > Use existing rte_get_systid() in RTE_LOG to print
> > thread_id.
> > > > >> > > > > > > >
> > > > >> > > > > > > > Changes in v5
> > > > >> > > > > > > > Rebase to latest code.
> > > > >> > > > > > > >
> > > > >> > > > > > > > Changes in v4
> > > > >> > > > > > > > None
> > > > >> > > > > > > >
> > > > >> > > > > > > > Changes in v3
> > > > >> > > > > > > > Changed subject to be more explicit on file name
> > inclusion.
> > > > >> > > > > > > >
> > > > >> > > > > > > > Changes in v2
> > > > >> > > > > > > > None
> > > > >> > > > > > > >
> > > > >> > > > > > > > Changes in v1
> > > > >> > > > > > > > eal_thread.c has minor differences between Linux and
> > BSD,
> > > > >> move
> > > > >> > > > > > > > entire file into common directory.
> > > > >> > > > > > > > Use RTE_EXEC_ENV_BSDAPP to differentiate on minor
> > > > >> differences.
> > > > >> > > > > > > > Rename eal_thread.c to eal_common_thread.c
> > > > >> > > > > > > > Makefile changes to reflect file move and name change.
> > > > >> > > > > > > > Fix checkpatch warnings.
> > > > >> > > > > > > >
> > > > >> > > > > > > > Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> > > > >> > > > > > > > ---
> > > > >> > > > > > > >  lib/librte_eal/bsdapp/eal/Makefile        |   2 +-
> > > > >> > > > > > > >  lib/librte_eal/bsdapp/eal/eal_thread.c    | 152
> > > > >> > > > > > > ------------------------------
> > > > >> > > > > > > >  lib/librte_eal/common/eal_common_thread.c | 147
> > > > >> > > > > > > ++++++++++++++++++++++++++++-
> > > > >> > > > > > > >  lib/librte_eal/linuxapp/eal/eal_thread.c  | 152
> > > > >> > > > > > > +-----------------------------
> > > > >> > > > > > > >  4 files changed, 148 insertions(+), 305 deletions(-)
> > > > >> > > > > > > >
> > > > >> > > > > > > > diff --git a/lib/librte_eal/bsdapp/eal/Makefile
> > > > >> > > > > > > b/lib/librte_eal/bsdapp/eal/Makefile
> > > > >> > > > > > > > index 2357cfa..55971b9 100644
> > > > >> > > > > > > > --- a/lib/librte_eal/bsdapp/eal/Makefile
> > > > >> > > > > > > > +++ b/lib/librte_eal/bsdapp/eal/Makefile
> > > > >> > > > > > > > @@ -87,7 +87,7 @@ CFLAGS_eal_common_log.o :=
> > -D_GNU_SOURCE
> > > > >> > > > > > > >  # workaround for a gcc bug with noreturn attribute
> > > > >> > > > > > > >  # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603
> > > > >> > > > > > > >  ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y)
> > > > >> > > > > > > > -CFLAGS_eal_thread.o += -Wno-return-type
> > > > >> > > > > > > > +CFLAGS_eal_common_thread.o += -Wno-return-type
> > > > >> > > > > > > >  CFLAGS_eal_hpet.o += -Wno-return-type
> > > > >> > > > > > > >  endif
> > > > >> > > > > > > >
> > > > >> > > > > > > > diff --git a/lib/librte_eal/bsdapp/eal/eal_thread.c
> > > > >> > > > > > > b/lib/librte_eal/bsdapp/eal/eal_thread.c
> > > > >> > > > > > > > index 9a03437..5714b8f 100644
> > > > >> > > > > > > > --- a/lib/librte_eal/bsdapp/eal/eal_thread.c
> > > > >> > > > > > > > +++ b/lib/librte_eal/bsdapp/eal/eal_thread.c
> > > > >> > > > > > > > @@ -35,163 +35,11 @@
> > > > >> > > > > > > >  #include <stdio.h>
> > > > >> > > > > > > >  #include <stdlib.h>
> > > > >> > > > > > > >  #include <stdint.h>
> > > > >> > > > > > > > -#include <unistd.h>
> > > > >> > > > > > > > -#include <sched.h>
> > > > >> > > > > > > > -#include <pthread_np.h>
> > > > >> > > > > > > > -#include <sys/queue.h>
> > > > >> > > > > > > >  #include <sys/thr.h>
> > > > >> > > > > > > >
> > > > >> > > > > > > > -#include <rte_debug.h>
> > > > >> > > > > > > > -#include <rte_atomic.h>
> > > > >> > > > > > > > -#include <rte_launch.h>
> > > > >> > > > > > > > -#include <rte_log.h>
> > > > >> > > > > > > > -#include <rte_memory.h>
> > > > >> > > > > > > > -#include <rte_memzone.h>
> > > > >> > > > > > > > -#include <rte_per_lcore.h>
> > > > >> > > > > > > > -#include <rte_eal.h>
> > > > >> > > > > > > > -#include <rte_per_lcore.h>
> > > > >> > > > > > > > -#include <rte_lcore.h>
> > > > >> > > > > > > > -
> > > > >> > > > > > > >  #include "eal_private.h"
> > > > >> > > > > > > >  #include "eal_thread.h"
> > > > >> > > > > > > >
> > > > >> > > > > > > > -RTE_DEFINE_PER_LCORE(unsigned, _lcore_id) =
> > LCORE_ID_ANY;
> > > > >> > > > > > > NAK, these are exported symbols, you can't remove them
> > without
> > > > >> > > going
> > > > >> > > > > > > through the
> > > > >> > > > > > > deprecation process.
> > > > >> > > > > > >
> > > > >> > > > > > >
> > > > >> > > > > > They are not removed/deleted, they are moved from
> > eal_thread.c
> > > > >> to
> > > > >> > > > > > eal_common_thread.c file since it is common to both Linux
> > and
> > > > >> BSD.
> > > > >> > > > > >
> > > > >> > > > > Then perhaps you forgot to export the symbol?  Its showing
> > up as
> > > > >> > > removed
> > > > >> > > > > on the
> > > > >> > > > > ABI checker utility.
> > > > >> > > > >
> > > > >> > > > > Neil
> > > > >> > > > >
> > > > >> > > >
> > > > >> > > > Can you please show me in the current code where it is being
> > > > >> exported? I
> > > > >> > > > have only moved definitions to _common_ files, not sure why it
> > > > >> should be
> > > > >> > > > exported now.  I searched in the current code for
> > > > >> RTE_DEFINE_PER_LCORE
> > > > >> > > >
> > > > >> > > > #home/rkerur/dpdk-tmp/dpdk# grep -ir RTE_DEFINE_PER_LCORE *
> > > > >> > > > app/test/test_per_lcore.c:static
> > RTE_DEFINE_PER_LCORE(unsigned,
> > > > >> test) =
> > > > >> > > > 0x12345678;
> > > > >> > > >
> > > > >>
> > lib/librte_eal/linuxapp/eal/eal_thread.c:RTE_DEFINE_PER_LCORE(unsigned,
> > > > >> > > > _lcore_id) = LCORE_ID_ANY;
> > > > >> > > >
> > > > >>
> > lib/librte_eal/linuxapp/eal/eal_thread.c:RTE_DEFINE_PER_LCORE(unsigned,
> > > > >> > > > _socket_id) = (unsigned)SOCKET_ID_ANY;
> > > > >> > > >
> > > > >> > >
> > > > >>
> > lib/librte_eal/linuxapp/eal/eal_thread.c:RTE_DEFINE_PER_LCORE(rte_cpuset_t,
> > > > >> > > > _cpuset);
> > > > >> > > >
> > > > >>
> > lib/librte_eal/bsdapp/eal/eal_thread.c:RTE_DEFINE_PER_LCORE(unsigned,
> > > > >> > > > _lcore_id) = LCORE_ID_ANY;
> > > > >> > > >
> > > > >>
> > lib/librte_eal/bsdapp/eal/eal_thread.c:RTE_DEFINE_PER_LCORE(unsigned,
> > > > >> > > > _socket_id) = (unsigned)SOCKET_ID_ANY;
> > > > >> > > >
> > > > >>
> > lib/librte_eal/bsdapp/eal/eal_thread.c:RTE_DEFINE_PER_LCORE(rte_cpuset_t,
> > > > >> > > > _cpuset);
> > > > >> > > > lib/librte_eal/common/include/rte_per_lcore.h:#define
> > > > >> > > > RTE_DEFINE_PER_LCORE(type, name)            \
> > > > >> > > > lib/librte_eal/common/include/rte_eal.h:    static
> > > > >> > > > RTE_DEFINE_PER_LCORE(int, _thread_id) = -1;
> > > > >> > > >
> > lib/librte_eal/common/eal_common_errno.c:RTE_DEFINE_PER_LCORE(int,
> > > > >> > > > _rte_errno);
> > > > >> > > > lib/librte_eal/common/eal_common_errno.c:    static
> > > > >> > > > RTE_DEFINE_PER_LCORE(char[RETVAL_SZ], retval);
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > > > Thanks
> > > > >> > > > > > Ravi
> > > > >> > > > > >
> > > > >> > > > > > Regards
> > > > >> > > > > > > Neil
> > > > >> > > > > > >
> > > > >> > > > > > >
> > > > >> > > > >
> > > > >> > > Its exported in the version map file:
> > > > >> > >  per_lcore__lcore_id;
> > > > >> > >
> > > > >> > >
> > > > >> > Thanks Neil, I checked and both linux and bsd rte_eal_version.map
> > have
> > > > >> it.
> > > > >> > I compared .map file between "changed code" and the original,
> > they are
> > > > >> same
> > > > >> > for both linux and bsd. In fact you had ACK'd v4 version of this
> > patch
> > > > >> > series and no major changes after that. Please let me know if I
> > missed
> > > > >> > something.
> > > > >> >
> > > > >> I did, and I'm retracting that, because I didn't think to check the
> > ABI
> > > > >> compatibility on this.  But I ran it throught the ABI checking
> > script
> > > > >> this and
> > > > >> this error popped out.  You should run it as well, its in the
> > scripts
> > > > >> directory.
> > > > >>
> > > > >>
> > > > >> I see in your first patch you removed it and re-added it in the
> > common
> > > > >> section.
> > > > >> But something about how its building is causing it to not show up
> > as an
> > > > >> exported
> > > > >> symbol, which is problematic, as other applications are going to
> > want
> > > > >> access to
> > > > >> it.
> > > > >>
> > > > >> It also possible that the ABI checker is throwing a false positive,
> > but
> > > > >> either
> > > > >> way, it needs to be looked into prior to moving forward with this.
> > > > >>
> > > > >>
> > > > > I did following things.
> > > > >
> > > > > Put a tag (v2.0.0-before-common-eal)  before EAL common functions
> > changes
> > > > > for commit (3c0c807038ad642f4be7deb9370293c39d12f029 net: remove
> > unneeded
> > > > > include)
> > > > >
> > > > > Put a tag (v2.0.0-common-eal) after EAL common functions changes for
> > > > > commit (25737e5a7212630a7b5d8ca756860a062f403789 Move common
> > functions in
> > > > > eal_pci.c)
> > > > >
> > > > > Ran validate-abi against x86_64-native-linuxapp-gcc and
> > > > >
> > > > > v2.0.0-rc3 and v2.0.0-before-common-eal, html report for
> > librte_eal.so
> > > > > shows removed symbols for "per_lcore__cpuset"
> > > > >
> > > > > v2.0.0-rc3 and v2.0.0-common-eal, html report for librte_eal.so shows
> > > > > removed symbols for "per_lcore__cpuset"
> > > > >
> > > > > Removed symbol is different from what you have reported and in my
> > case I
> > > > > see it even before my commit. If you are interested I can unicast
> > you html
> > > > > report file. Please let me know how to proceed.
> > > > >
> > > > >
> > > >
> > > > I did some experiment and found some interesting things.  I will take
> > eal.c
> > > > as an example
> > > >
> > > > eal.c is split into eal_common_sysfs.c eal_common_mem_cfg.c
> > > > eal_common_proc_type.c and eal_common_app_usage.c. In
> > linuxapp/eal/Makefile
> > > > if I compile new files right after eal.c as shown below
> > > >
> > > > SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) := eal.c
> > > > SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_sysfs.c
> > > > SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_mem_cfg.c
> > > > SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_proc_type.c
> > > > SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_app_usage.c
> > > > ...
> > > >
> > > > validate-abi results matches baseline. Instead if i place new _common_
> > > > files in common area in linuxapp/eal/Makefile as shown below
> > > >
> > > > # from common dir
> > > > SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_memzone.c
> > > > SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_log.c
> > > > ...
> > > >
> > > > validate-abi reports problem in binary compatibility and source
> > > > compatiblity
> > > >
> > > > eal_filesystem.h, librte_eal.so.1
> > > >  [+] eal_parse_sysfs_value ( char const* filename, unsigned long* val )
> > > >  @@ DPDK_2.0 (2)
> > > >
> > > > I believe files in common and linuxapp directory are compiled same way
> > so
> > > > not sure why placement in makefile makes difference.
> > > >
> > > > Could this be false-positive from validate-abi script??
> > > >
> > > It could be, yes.  Though I'm more inclined to think that perhaps in the
> > new
> > > version of the code we're not generating ithe same dwarf information out
> > of it.
> > > In fact for some reason, I've checked both the build before and after
> > your
> > > patch series, and the exported CFLAGS aren't getting passed to the build
> > > properly, implying that we're not building all the code in the validator
> > with
> > > the -g flag, which the validator need to function properly.  I'm looking
> > into
> > > that
> > > Neil
> > >
> > >
> > Found the problem, I was stupidly reading the report incorrectly.  The
> > problem
> > regarding _lcore_id is a source compatibilty issue (because the symbol
> > moved to
> > a new location), which is irrelevant to us.  Its not in any way a binary
> > compat
> > problem, which is what we care about.  Sorry for the noise.
> >
> > I do still have a few concerns about some changed calling conventions with
> > a few
> > other functions, which I'll look into on monday.
> >
> >
> Please let me know your inputs on changed calling conventions. Most of them
> can be fixed by re-arranging moved code in _common_ files and order of
> compilation.
> 
If moving the order of compliation around fixes the problem, then I am
reasonably convinced that it is, if not a false positive, a minor issue with the
compilers dwarf information (The compiler just can't sanely change the location
in which parameters are passed).  If you make those changes, I'll ACK them, and
look into whats going on with the calling conventions

Thanks!
Neil

> Thanks,
> Ravi
> 
> Regards
> > Neil
> >
> >