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

  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(-)
  

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.

Regards
Neil
  

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.

Thanks
Ravi

Regards
> Neil
>
>
  

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

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

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

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;

Neil
  

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.

dpdk-common-eal-f-4 has all the common code movement changes
dpdk-tmp is original code

#/home/rkerur/dpdk-common-eal-f-4/dpdk# diff
lib/librte_eal/linuxapp/eal/rte_eal_version.map
/home/rkerur/dpdk-tmp/dpdk/lib/librte_eal/linuxapp/eal/rte_eal_version.map

<No difference>

#/home/rkerur/dpdk-common-eal-f-4/dpdk# diff
lib/librte_eal/bsdapp/eal/rte_eal_version.map
/home/rkerur/dpdk-tmp/dpdk/lib/librte_eal/bsdapp/eal/rte_eal_version.map

<No difference>

Thanks,
Ravi

Neil
>
>
  

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.

Neil

> dpdk-common-eal-f-4 has all the common code movement changes
> dpdk-tmp is original code
> 
> #/home/rkerur/dpdk-common-eal-f-4/dpdk# diff
> lib/librte_eal/linuxapp/eal/rte_eal_version.map
> /home/rkerur/dpdk-tmp/dpdk/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> 
> <No difference>
> 
> #/home/rkerur/dpdk-common-eal-f-4/dpdk# diff
> lib/librte_eal/bsdapp/eal/rte_eal_version.map
> /home/rkerur/dpdk-tmp/dpdk/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> 
> <No difference>
> 
> Thanks,
> Ravi
> 
> Neil
> >
> >
  

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.


> Neil
>
> > dpdk-common-eal-f-4 has all the common code movement changes
> > dpdk-tmp is original code
> >
> > #/home/rkerur/dpdk-common-eal-f-4/dpdk# diff
> > lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >
> /home/rkerur/dpdk-tmp/dpdk/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >
> > <No difference>
> >
> > #/home/rkerur/dpdk-common-eal-f-4/dpdk# diff
> > lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > /home/rkerur/dpdk-tmp/dpdk/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> >
> > <No difference>
> >
> > Thanks,
> > Ravi
> >
> > Neil
> > >
> > >
>
  

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

> Neil
>>
>> > dpdk-common-eal-f-4 has all the common code movement changes
>> > dpdk-tmp is original code
>> >
>> > #/home/rkerur/dpdk-common-eal-f-4/dpdk# diff
>> > lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> >
>> /home/rkerur/dpdk-tmp/dpdk/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> >
>> > <No difference>
>> >
>> > #/home/rkerur/dpdk-common-eal-f-4/dpdk# diff
>> > lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > /home/rkerur/dpdk-tmp/dpdk/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> >
>> > <No difference>
>> >
>> > Thanks,
>> > Ravi
>> >
>> > Neil
>> > >
>> > >
>>
>
>
  

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
  

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.

Regards
Neil
  

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.

Thanks,
Ravi

Regards
> Neil
>
>
  

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

On Mon, Apr 27, 2015 at 6:44 AM, Neil Horman <nhorman@tuxdriver.com> wrote:

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

Issues like the one shown below are taken care by reordering the code
compilation.

eal_parse_sysfs_value ( char const* filename, unsigned long* val )

Change
The parameter filename became passed on stack instead of rdi register

Effect
Violation of the calling convention. This may result in crash or incorrect
behavior of applications.

Last one that is left out is in

rte_thread_set_affinity ( rte_cpuset_t* p1 )

Change
The parameter *p1* became passed in *rdi* register instead of stack.

Effect
Violation of the calling convention. This may result in crash or incorrect
behavior of applications.

After checking abi-0.99.pdf (x86-64.org) looks like for
"rte_thread_set_affinity" new code is doing the right thing by passing the
parameter in "rdi" register since pointer is classified as "integer_class".
Nothing needs to be fixed here. After you confirm that warning can be
ignored I will work on sending new revision.

Thanks,
Ravi


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

On Mon, Apr 27, 2015 at 03:39:41PM -0700, Ravi Kerur wrote:
> On Mon, Apr 27, 2015 at 6:44 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > 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
> >
> 
> Issues like the one shown below are taken care by reordering the code
> compilation.
> 
> eal_parse_sysfs_value ( char const* filename, unsigned long* val )
> 
> Change
> The parameter filename became passed on stack instead of rdi register
> 
> Effect
> Violation of the calling convention. This may result in crash or incorrect
> behavior of applications.
> 
> Last one that is left out is in
> 
> rte_thread_set_affinity ( rte_cpuset_t* p1 )
> 
> Change
> The parameter *p1* became passed in *rdi* register instead of stack.
> 
> Effect
> Violation of the calling convention. This may result in crash or incorrect
> behavior of applications.
> 
> After checking abi-0.99.pdf (x86-64.org) looks like for
> "rte_thread_set_affinity" new code is doing the right thing by passing the
> parameter in "rdi" register since pointer is classified as "integer_class".
> Nothing needs to be fixed here. After you confirm that warning can be
> ignored I will work on sending new revision.
> 
ACK then, send the new revision, this appears to be a false positive.

Thanks for taking the time to confirm.

Best
Neil

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

On Tue, Apr 28, 2015 at 12:35 PM, Neil Horman <nhorman@tuxdriver.com> wrote:

> On Mon, Apr 27, 2015 at 03:39:41PM -0700, Ravi Kerur wrote:
> > On Mon, Apr 27, 2015 at 6:44 AM, Neil Horman <nhorman@tuxdriver.com>
> wrote:
> >
> > > 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
> > >
> >
> > Issues like the one shown below are taken care by reordering the code
> > compilation.
> >
> > eal_parse_sysfs_value ( char const* filename, unsigned long* val )
> >
> > Change
> > The parameter filename became passed on stack instead of rdi register
> >
> > Effect
> > Violation of the calling convention. This may result in crash or
> incorrect
> > behavior of applications.
> >
> > Last one that is left out is in
> >
> > rte_thread_set_affinity ( rte_cpuset_t* p1 )
> >
> > Change
> > The parameter *p1* became passed in *rdi* register instead of stack.
> >
> > Effect
> > Violation of the calling convention. This may result in crash or
> incorrect
> > behavior of applications.
> >
> > After checking abi-0.99.pdf (x86-64.org) looks like for
> > "rte_thread_set_affinity" new code is doing the right thing by passing
> the
> > parameter in "rdi" register since pointer is classified as
> "integer_class".
> > Nothing needs to be fixed here. After you confirm that warning can be
> > ignored I will work on sending new revision.
> >
> ACK then, send the new revision, this appears to be a false positive.
>
> Thanks for taking the time to confirm.
>

Thanks Neil. I have sent v8 which fixes ABI warnings. I have tested it with
x86_64-native-linuxapp-gcc, x86_64-native-linuxapp-clang and
x86_64-ivshmem-gcc targets. ABI results look fine to me.

I tried to run validate-abi.sh on BSD but ran into errors. If there is a
way to check against BSD please let me know.

>
> Best
> Neil
>
> > Thanks,
> > Ravi
> >
> >
> > > Thanks!
> > > Neil
> > >
> > > > Thanks,
> > > > Ravi
> > > >
> > > > Regards
> > > > > Neil
> > > > >
> > > > >
> > >
>
  

On Tue, Apr 28, 2015 at 04:52:37PM -0700, Ravi Kerur wrote:
> On Tue, Apr 28, 2015 at 12:35 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Mon, Apr 27, 2015 at 03:39:41PM -0700, Ravi Kerur wrote:
> > > On Mon, Apr 27, 2015 at 6:44 AM, Neil Horman <nhorman@tuxdriver.com>
> > wrote:
> > >
> > > > 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
> > > >
> > >
> > > Issues like the one shown below are taken care by reordering the code
> > > compilation.
> > >
> > > eal_parse_sysfs_value ( char const* filename, unsigned long* val )
> > >
> > > Change
> > > The parameter filename became passed on stack instead of rdi register
> > >
> > > Effect
> > > Violation of the calling convention. This may result in crash or
> > incorrect
> > > behavior of applications.
> > >
> > > Last one that is left out is in
> > >
> > > rte_thread_set_affinity ( rte_cpuset_t* p1 )
> > >
> > > Change
> > > The parameter *p1* became passed in *rdi* register instead of stack.
> > >
> > > Effect
> > > Violation of the calling convention. This may result in crash or
> > incorrect
> > > behavior of applications.
> > >
> > > After checking abi-0.99.pdf (x86-64.org) looks like for
> > > "rte_thread_set_affinity" new code is doing the right thing by passing
> > the
> > > parameter in "rdi" register since pointer is classified as
> > "integer_class".
> > > Nothing needs to be fixed here. After you confirm that warning can be
> > > ignored I will work on sending new revision.
> > >
> > ACK then, send the new revision, this appears to be a false positive.
> >
> > Thanks for taking the time to confirm.
> >
> 
> Thanks Neil. I have sent v8 which fixes ABI warnings. I have tested it with
> x86_64-native-linuxapp-gcc, x86_64-native-linuxapp-clang and
> x86_64-ivshmem-gcc targets. ABI results look fine to me.
> 
> I tried to run validate-abi.sh on BSD but ran into errors. If there is a
> way to check against BSD please let me know.
> 
The ABI checker should work on BSD as far as I know, since it only relies on
dwarf information in the output binary.  What errors are you seeing?

Neil
  

> > I tried to run validate-abi.sh on BSD but ran into errors. If there is a
> > way to check against BSD please let me know.
> >
> The ABI checker should work on BSD as far as I know, since it only relies
> on
> dwarf information in the output binary.  What errors are you seeing?
>

dpdk-bsd:/home/rkerur/dpdk-validate-abi-1/dpdk # sh
./scripts/validate-abi.sh v2.0.0-rc3 v2.0.0-abi x86_64-native-bsdapp-clang
mktemp: illegal option -- p
usage: mktemp [-d] [-q] [-t prefix] [-u] template ...
       mktemp [-d] [-q] [-u] -t prefix
Cant find abi-compliance-checker utility

abi-compliance-checker is installed as shown below.

dpdk-bsd:/home/rkerur/dpdk-validate-abi-1/dpdk # pkg install
devel/abi-compliance-checker
Updating FreeBSD repository catalogue...
FreeBSD repository is up-to-date.
All repositories are up-to-date.
Checking integrity... done (0 conflicting)
The most recent version of packages are already installed


>
> Neil
>
>
  

On Wed, Apr 29, 2015 at 10:47:04AM -0700, Ravi Kerur wrote:
> > > I tried to run validate-abi.sh on BSD but ran into errors. If there is a
> > > way to check against BSD please let me know.
> > >
> > The ABI checker should work on BSD as far as I know, since it only relies
> > on
> > dwarf information in the output binary.  What errors are you seeing?
> >
> 
> dpdk-bsd:/home/rkerur/dpdk-validate-abi-1/dpdk # sh
> ./scripts/validate-abi.sh v2.0.0-rc3 v2.0.0-abi x86_64-native-bsdapp-clang
> mktemp: illegal option -- p
Ah, bsd mktemp doesn't support the -p option.  I'll see if I can fix that.

> usage: mktemp [-d] [-q] [-t prefix] [-u] template ...
>        mktemp [-d] [-q] [-u] -t prefix
> Cant find abi-compliance-checker utility
> 
> abi-compliance-checker is installed as shown below.
> 
> dpdk-bsd:/home/rkerur/dpdk-validate-abi-1/dpdk # pkg install
> devel/abi-compliance-checker
> Updating FreeBSD repository catalogue...
> FreeBSD repository is up-to-date.
> All repositories are up-to-date.
> Checking integrity... done (0 conflicting)
> The most recent version of packages are already installed
> 

Whats the path for abi-compliance checker there?  It would seem that the binary
isn't in your path, as which isn't locating it.
Neil

> 
> >
> > Neil
> >
> >
  

On Thu, Apr 30, 2015 at 9:00 AM, Neil Horman <nhorman@tuxdriver.com> wrote:

> On Wed, Apr 29, 2015 at 10:47:04AM -0700, Ravi Kerur wrote:
> > > > I tried to run validate-abi.sh on BSD but ran into errors. If there
> is a
> > > > way to check against BSD please let me know.
> > > >
> > > The ABI checker should work on BSD as far as I know, since it only
> relies
> > > on
> > > dwarf information in the output binary.  What errors are you seeing?
> > >
> >
> > dpdk-bsd:/home/rkerur/dpdk-validate-abi-1/dpdk # sh
> > ./scripts/validate-abi.sh v2.0.0-rc3 v2.0.0-abi
> x86_64-native-bsdapp-clang
> > mktemp: illegal option -- p
> Ah, bsd mktemp doesn't support the -p option.  I'll see if I can fix that.
>

I think there are couple of other issues I found

freeBSD sed is different from Linux (GNU sed) and I get following errors
with the script

"sed 1 command c expects \ followed by text".

I have to use gsed (GNU sed) in freeBSD to get rid of that error and
similarly freeBSD uses gmake instead of make.  I have made those minor
changes and sending them with this email as an attachment.


> > usage: mktemp [-d] [-q] [-t prefix] [-u] template ...
> >        mktemp [-d] [-q] [-u] -t prefix
> > Cant find abi-compliance-checker utility
> >
> > abi-compliance-checker is installed as shown below.
> >
> > dpdk-bsd:/home/rkerur/dpdk-validate-abi-1/dpdk # pkg install
> > devel/abi-compliance-checker
> > Updating FreeBSD repository catalogue...
> > FreeBSD repository is up-to-date.
> > All repositories are up-to-date.
> > Checking integrity... done (0 conflicting)
> > The most recent version of packages are already installed
> >
>
> Whats the path for abi-compliance checker there?  It would seem that the
> binary
> isn't in your path, as which isn't locating it.
>

I am using regular freeBSD port install which doesn't install in any
/usr/bin or /usr/local/bin. I finally decided to install both abi-dumper
and abi-compliance-checker from source, compile and install it in correct
directory. Above error is fixed after that, however, abi utilities use
"eu-readelf" and I can't find that utility to install in freeBSD. I get
following errors

ERROR: can't find "eu-readelf" command

freeBSD has only readelf. Please let me know if there is a way to get rid
of this error.

Thanks,
Ravi

>
> > >
> > > Neil
> > >
> > >
>