[v1,1/5] mempool: remove internal tracepoints from version map
Checks
Commit Message
The file rte_mempool_trace.h contains tracepoints which are internal to the
mempool library. This file is renamed to mempool_trace.h, and is made an
internal header. The tracepoints in this file are removed from the
experimental section in version.map file.
Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
.../{rte_mempool_trace.h => mempool_trace.h} | 6 +++---
lib/mempool/mempool_trace_points.c | 2 +-
lib/mempool/meson.build | 4 +++-
lib/mempool/rte_mempool.c | 2 +-
lib/mempool/rte_mempool_ops.c | 2 +-
lib/mempool/version.map | 14 --------------
6 files changed, 9 insertions(+), 21 deletions(-)
rename lib/mempool/{rte_mempool_trace.h => mempool_trace.h} (98%)
Comments
On Thu, Feb 9, 2023 at 2:30 PM Ankur Dwivedi <adwivedi@marvell.com> wrote:
>
> The file rte_mempool_trace.h contains tracepoints which are internal to the
> mempool library. This file is renamed to mempool_trace.h, and is made an
> internal header. The tracepoints in this file are removed from the
This patch also exports this new internal header which looks wrong to me.
See below.
> experimental section in version.map file.
>
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> diff --git a/lib/mempool/meson.build b/lib/mempool/meson.build
> index b8aaa00694..29ae6d21e5 100644
> --- a/lib/mempool/meson.build
> +++ b/lib/mempool/meson.build
> @@ -17,7 +17,9 @@ sources = files(
> )
> headers = files(
> 'rte_mempool.h',
> - 'rte_mempool_trace.h',
> 'rte_mempool_trace_fp.h',
> )
> +driver_sdk_headers += files(
> + 'mempool_trace.h',
> +)
driver_sdk_headers is for exporting driver headers.
I am not sure why you added this, can you elaborate?
Checking with who includes this header in the whole tree:
$ git grep include..mempool_trace
lib/mempool/mempool_trace_points.c:#include "mempool_trace.h"
lib/mempool/rte_mempool.c:#include "mempool_trace.h"
lib/mempool/rte_mempool_ops.c:#include "mempool_trace.h"
I see no external (from the mempool library pov) user of this header
=> no export needed.
I did not check the rest of the series, but the same argument is
likely to apply.
Thanks.
>On Thu, Feb 9, 2023 at 2:30 PM Ankur Dwivedi <adwivedi@marvell.com>
>wrote:
>>
>> The file rte_mempool_trace.h contains tracepoints which are internal
>> to the mempool library. This file is renamed to mempool_trace.h, and
>> is made an internal header. The tracepoints in this file are removed
>> from the
>
>This patch also exports this new internal header which looks wrong to me.
>See below.
>
>
>> experimental section in version.map file.
>>
>> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>
>> diff --git a/lib/mempool/meson.build b/lib/mempool/meson.build index
>> b8aaa00694..29ae6d21e5 100644
>> --- a/lib/mempool/meson.build
>> +++ b/lib/mempool/meson.build
>> @@ -17,7 +17,9 @@ sources = files(
>> )
>> headers = files(
>> 'rte_mempool.h',
>> - 'rte_mempool_trace.h',
>> 'rte_mempool_trace_fp.h',
>> )
>> +driver_sdk_headers += files(
>> + 'mempool_trace.h',
>> +)
>
>driver_sdk_headers is for exporting driver headers.
Ok.
>I am not sure why you added this, can you elaborate?
I saw the changes done in lib/eventdev for eventdev_trace.h. Made similar changes for
mempool and other library.
>
>Checking with who includes this header in the whole tree:
>$ git grep include..mempool_trace
>lib/mempool/mempool_trace_points.c:#include "mempool_trace.h"
>lib/mempool/rte_mempool.c:#include "mempool_trace.h"
>lib/mempool/rte_mempool_ops.c:#include "mempool_trace.h"
>
>I see no external (from the mempool library pov) user of this header => no
>export needed.
Ok, will remove it.
>
>I did not check the rest of the series, but the same argument is likely to apply.
>
>
>Thanks.
>
>--
>David Marchand
On Thu, Feb 9, 2023 at 7:00 PM Ankur Dwivedi <adwivedi@marvell.com> wrote:
>
> The file rte_mempool_trace.h contains tracepoints which are internal to the
> mempool library. This file is renamed to mempool_trace.h, and is made an
> internal header. The tracepoints in this file are removed from the
> experimental section in version.map file.
>
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> @@ -47,22 +47,8 @@ EXPERIMENTAL {
> __rte_mempool_trace_generic_get;
> __rte_mempool_trace_get_bulk;
> __rte_mempool_trace_get_contig_blocks;
I think, FP ones also can be removed.
Also, in one of the patch(may be eal). Update the doc
https://doc.dpdk.org/guides/prog_guide/trace_lib.html
The following note can be removed:
The RTE_TRACE_POINT_REGISTER defines the placeholder for the
rte_trace_point_t tracepoint object. The user must export a
__<trace_function_name> symbol in the library .map file for this
tracepoint to be used out of the library, in shared builds. For
example, __app_trace_string will be the exported symbol in the above
example.
> - __rte_mempool_trace_create;
> - __rte_mempool_trace_create_empty;
> - __rte_mempool_trace_free;
> - __rte_mempool_trace_populate_iova;
> - __rte_mempool_trace_populate_virt;
> - __rte_mempool_trace_populate_default;
> - __rte_mempool_trace_populate_anon;
> - __rte_mempool_trace_cache_create;
> - __rte_mempool_trace_cache_free;
> __rte_mempool_trace_default_cache;
> - __rte_mempool_trace_get_page_size;
> __rte_mempool_trace_cache_flush;
> - __rte_mempool_trace_ops_populate;
> - __rte_mempool_trace_ops_alloc;
> - __rte_mempool_trace_ops_free;
> - __rte_mempool_trace_set_ops_byname;
> };
>
> INTERNAL {
> --
> 2.25.1
>
>On Thu, Feb 9, 2023 at 7:00 PM Ankur Dwivedi <adwivedi@marvell.com>
>wrote:
>>
>> The file rte_mempool_trace.h contains tracepoints which are internal
>> to the mempool library. This file is renamed to mempool_trace.h, and
>> is made an internal header. The tracepoints in this file are removed
>> from the experimental section in version.map file.
>>
>> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>
>> @@ -47,22 +47,8 @@ EXPERIMENTAL {
>> __rte_mempool_trace_generic_get;
>> __rte_mempool_trace_get_bulk;
>> __rte_mempool_trace_get_contig_blocks;
>
>I think, FP ones also can be removed.
The FP symbols are used in header file rte_mempool.h. Removing the symbols will cause build
failure in shared build.
>
>Also, in one of the patch(may be eal). Update the doc
>https://urldefense.proofpoint.com/v2/url?u=https-
>3A__doc.dpdk.org_guides_prog-5Fguide_trace-
>5Flib.html&d=DwIBaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=ILjiNF3GF25y6QdHZU
>xMl6JrStU0MIuCtO5dMzn3Ybk&m=wr975-
>STqC3Y1eiGa9KZMdPdNKingEIEfZXvMkri8VO0p31eWWzr8kAoQC2TEIxV&s=g_
>M3SEGmHvisHLnOLO8ilDQbVQ85MFRR2YCmCfYZ3SE&e=
>
>The following note can be removed:
>
>The RTE_TRACE_POINT_REGISTER defines the placeholder for the
>rte_trace_point_t tracepoint object. The user must export a
>__<trace_function_name> symbol in the library .map file for this tracepoint to
>be used out of the library, in shared builds. For example, __app_trace_string
>will be the exported symbol in the above example.
>
>
>
>> - __rte_mempool_trace_create;
>> - __rte_mempool_trace_create_empty;
>> - __rte_mempool_trace_free;
>> - __rte_mempool_trace_populate_iova;
>> - __rte_mempool_trace_populate_virt;
>> - __rte_mempool_trace_populate_default;
>> - __rte_mempool_trace_populate_anon;
>> - __rte_mempool_trace_cache_create;
>> - __rte_mempool_trace_cache_free;
>> __rte_mempool_trace_default_cache;
>> - __rte_mempool_trace_get_page_size;
>> __rte_mempool_trace_cache_flush;
>> - __rte_mempool_trace_ops_populate;
>> - __rte_mempool_trace_ops_alloc;
>> - __rte_mempool_trace_ops_free;
>> - __rte_mempool_trace_set_ops_byname;
>> };
>>
>> INTERNAL {
>> --
>> 2.25.1
>>
On Fri, Feb 10, 2023 at 12:30 PM Ankur Dwivedi <adwivedi@marvell.com> wrote:
>
> >On Thu, Feb 9, 2023 at 7:00 PM Ankur Dwivedi <adwivedi@marvell.com>
> >wrote:
> >>
> >> The file rte_mempool_trace.h contains tracepoints which are internal
> >> to the mempool library. This file is renamed to mempool_trace.h, and
> >> is made an internal header. The tracepoints in this file are removed
> >> from the experimental section in version.map file.
> >>
> >> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> >
> >> @@ -47,22 +47,8 @@ EXPERIMENTAL {
> >> __rte_mempool_trace_generic_get;
> >> __rte_mempool_trace_get_bulk;
> >> __rte_mempool_trace_get_contig_blocks;
> >
> >I think, FP ones also can be removed.
>
> The FP symbols are used in header file rte_mempool.h. Removing the symbols will cause build
> failure in shared build.
OK. Please update the below note documentation only for FP symbols then.
> >
> >Also, in one of the patch(may be eal). Update the doc
> >https://urldefense.proofpoint.com/v2/url?u=https-
> >3A__doc.dpdk.org_guides_prog-5Fguide_trace-
> >5Flib.html&d=DwIBaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=ILjiNF3GF25y6QdHZU
> >xMl6JrStU0MIuCtO5dMzn3Ybk&m=wr975-
> >STqC3Y1eiGa9KZMdPdNKingEIEfZXvMkri8VO0p31eWWzr8kAoQC2TEIxV&s=g_
> >M3SEGmHvisHLnOLO8ilDQbVQ85MFRR2YCmCfYZ3SE&e=
> >
> >The following note can be removed:
> >
> >The RTE_TRACE_POINT_REGISTER defines the placeholder for the
> >rte_trace_point_t tracepoint object. The user must export a
> >__<trace_function_name> symbol in the library .map file for this tracepoint to
> >be used out of the library, in shared builds. For example, __app_trace_string
> >will be the exported symbol in the above example.
> >
> >
> >
> >> - __rte_mempool_trace_create;
> >> - __rte_mempool_trace_create_empty;
> >> - __rte_mempool_trace_free;
> >> - __rte_mempool_trace_populate_iova;
> >> - __rte_mempool_trace_populate_virt;
> >> - __rte_mempool_trace_populate_default;
> >> - __rte_mempool_trace_populate_anon;
> >> - __rte_mempool_trace_cache_create;
> >> - __rte_mempool_trace_cache_free;
> >> __rte_mempool_trace_default_cache;
> >> - __rte_mempool_trace_get_page_size;
> >> __rte_mempool_trace_cache_flush;
> >> - __rte_mempool_trace_ops_populate;
> >> - __rte_mempool_trace_ops_alloc;
> >> - __rte_mempool_trace_ops_free;
> >> - __rte_mempool_trace_set_ops_byname;
> >> };
> >>
> >> INTERNAL {
> >> --
> >> 2.25.1
> >>
On Fri, Feb 10, 2023 at 8:06 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Fri, Feb 10, 2023 at 12:30 PM Ankur Dwivedi <adwivedi@marvell.com> wrote:
> >
> > >On Thu, Feb 9, 2023 at 7:00 PM Ankur Dwivedi <adwivedi@marvell.com>
> > >wrote:
> > >>
> > >> The file rte_mempool_trace.h contains tracepoints which are internal
> > >> to the mempool library. This file is renamed to mempool_trace.h, and
> > >> is made an internal header. The tracepoints in this file are removed
> > >> from the experimental section in version.map file.
> > >>
> > >> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> > >
> > >> @@ -47,22 +47,8 @@ EXPERIMENTAL {
> > >> __rte_mempool_trace_generic_get;
> > >> __rte_mempool_trace_get_bulk;
> > >> __rte_mempool_trace_get_contig_blocks;
> > >
> > >I think, FP ones also can be removed.
> >
> > The FP symbols are used in header file rte_mempool.h. Removing the symbols will cause build
> > failure in shared build.
>
> OK. Please update the below note documentation only for FP symbols then.
I disagree.
We may enhance this note, but it is not a matter of being FP / non-FP.
A simple example is the "generic" non-FP tracepoints provided by EAL.
On Fri, Feb 10, 2023 at 7:54 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Feb 10, 2023 at 8:06 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Fri, Feb 10, 2023 at 12:30 PM Ankur Dwivedi <adwivedi@marvell.com> wrote:
> > >
> > > >On Thu, Feb 9, 2023 at 7:00 PM Ankur Dwivedi <adwivedi@marvell.com>
> > > >wrote:
> > > >>
> > > >> The file rte_mempool_trace.h contains tracepoints which are internal
> > > >> to the mempool library. This file is renamed to mempool_trace.h, and
> > > >> is made an internal header. The tracepoints in this file are removed
> > > >> from the experimental section in version.map file.
> > > >>
> > > >> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> > > >
> > > >> @@ -47,22 +47,8 @@ EXPERIMENTAL {
> > > >> __rte_mempool_trace_generic_get;
> > > >> __rte_mempool_trace_get_bulk;
> > > >> __rte_mempool_trace_get_contig_blocks;
> > > >
> > > >I think, FP ones also can be removed.
> > >
> > > The FP symbols are used in header file rte_mempool.h. Removing the symbols will cause build
> > > failure in shared build.
> >
> > OK. Please update the below note documentation only for FP symbols then.
>
> I disagree.
>
> We may enhance this note, but it is not a matter of being FP / non-FP.
> A simple example is the "generic" non-FP tracepoints provided by EAL.
OK. Then what's your recommendation for the document update ?
Generic datatype exposed by EAL trace library or FP trace point
object. or something else?
I am trying to see what needs to be added/changed in documentation?
>
>
> --
> David Marchand
>
>Subject: Re: [EXT] Re: [PATCH v1 1/5] mempool: remove internal tracepoints
>from version map
>
>On Fri, Feb 10, 2023 at 7:54 PM David Marchand
><david.marchand@redhat.com> wrote:
>>
>> On Fri, Feb 10, 2023 at 8:06 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>> >
>> > On Fri, Feb 10, 2023 at 12:30 PM Ankur Dwivedi <adwivedi@marvell.com>
>wrote:
>> > >
>> > > >On Thu, Feb 9, 2023 at 7:00 PM Ankur Dwivedi
>> > > ><adwivedi@marvell.com>
>> > > >wrote:
>> > > >>
>> > > >> The file rte_mempool_trace.h contains tracepoints which are
>> > > >> internal to the mempool library. This file is renamed to
>> > > >> mempool_trace.h, and is made an internal header. The
>> > > >> tracepoints in this file are removed from the experimental section in
>version.map file.
>> > > >>
>> > > >> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>> > > >
>> > > >> @@ -47,22 +47,8 @@ EXPERIMENTAL {
>> > > >> __rte_mempool_trace_generic_get;
>> > > >> __rte_mempool_trace_get_bulk;
>> > > >> __rte_mempool_trace_get_contig_blocks;
>> > > >
>> > > >I think, FP ones also can be removed.
>> > >
>> > > The FP symbols are used in header file rte_mempool.h. Removing the
>> > > symbols will cause build failure in shared build.
>> >
>> > OK. Please update the below note documentation only for FP symbols
>then.
>>
>> I disagree.
>>
>> We may enhance this note, but it is not a matter of being FP / non-FP.
>> A simple example is the "generic" non-FP tracepoints provided by EAL.
>
>OK. Then what's your recommendation for the document update ?
>Generic datatype exposed by EAL trace library or FP trace point object. or
>something else?
>I am trying to see what needs to be added/changed in documentation?
Will the following line in document cover the different cases:
For generic tracepoint or for tracepoint used in public header files, the user must export a ``__<trace_function_name>`` symbol in the library ``.map`` file, for this tracepoint to be used out of the library in shared builds.
>
>
>
>>
>>
>> --
>> David Marchand
>>
On Tue, Feb 14, 2023 at 12:37 PM Ankur Dwivedi <adwivedi@marvell.com> wrote:
>
> >Subject: Re: [EXT] Re: [PATCH v1 1/5] mempool: remove internal tracepoints
> >from version map
> >
> >On Fri, Feb 10, 2023 at 7:54 PM David Marchand
> ><david.marchand@redhat.com> wrote:
> >>
> >> On Fri, Feb 10, 2023 at 8:06 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >> >
> >> > On Fri, Feb 10, 2023 at 12:30 PM Ankur Dwivedi <adwivedi@marvell.com>
> >wrote:
> >> > >
> >> > > >On Thu, Feb 9, 2023 at 7:00 PM Ankur Dwivedi
> >> > > ><adwivedi@marvell.com>
> >> > > >wrote:
> >> > > >>
> >> > > >> The file rte_mempool_trace.h contains tracepoints which are
> >> > > >> internal to the mempool library. This file is renamed to
> >> > > >> mempool_trace.h, and is made an internal header. The
> >> > > >> tracepoints in this file are removed from the experimental section in
> >version.map file.
> >> > > >>
> >> > > >> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> >> > > >
> >> > > >> @@ -47,22 +47,8 @@ EXPERIMENTAL {
> >> > > >> __rte_mempool_trace_generic_get;
> >> > > >> __rte_mempool_trace_get_bulk;
> >> > > >> __rte_mempool_trace_get_contig_blocks;
> >> > > >
> >> > > >I think, FP ones also can be removed.
> >> > >
> >> > > The FP symbols are used in header file rte_mempool.h. Removing the
> >> > > symbols will cause build failure in shared build.
> >> >
> >> > OK. Please update the below note documentation only for FP symbols
> >then.
> >>
> >> I disagree.
> >>
> >> We may enhance this note, but it is not a matter of being FP / non-FP.
> >> A simple example is the "generic" non-FP tracepoints provided by EAL.
> >
> >OK. Then what's your recommendation for the document update ?
> >Generic datatype exposed by EAL trace library or FP trace point object. or
> >something else?
> >I am trying to see what needs to be added/changed in documentation?
>
> Will the following line in document cover the different cases:
>
> For generic tracepoint or for tracepoint used in public header files, the user must export a ``__<trace_function_name>`` symbol in the library ``.map`` file, for this tracepoint to be used out of the library in shared builds.
Yes, this sounds ok to me.
Thanks for updating in v3.
similarity index 98%
rename from lib/mempool/rte_mempool_trace.h
rename to lib/mempool/mempool_trace.h
@@ -2,8 +2,8 @@
* Copyright(C) 2020 Marvell International Ltd.
*/
-#ifndef _RTE_MEMPOOL_TRACE_H_
-#define _RTE_MEMPOOL_TRACE_H_
+#ifndef _MEMPOOL_TRACE_H_
+#define _MEMPOOL_TRACE_H_
/**
* @file
@@ -172,4 +172,4 @@ RTE_TRACE_POINT(
}
#endif
-#endif /* _RTE_MEMPOOL_TRACE_H_ */
+#endif /* _MEMPOOL_TRACE_H_ */
@@ -4,7 +4,7 @@
#include <rte_trace_point_register.h>
-#include "rte_mempool_trace.h"
+#include "mempool_trace.h"
RTE_TRACE_POINT_REGISTER(rte_mempool_trace_ops_dequeue_bulk,
lib.mempool.ops.deq.bulk)
@@ -17,7 +17,9 @@ sources = files(
)
headers = files(
'rte_mempool.h',
- 'rte_mempool_trace.h',
'rte_mempool_trace_fp.h',
)
+driver_sdk_headers += files(
+ 'mempool_trace.h',
+)
deps += ['ring', 'telemetry']
@@ -28,8 +28,8 @@
#include <rte_eal_paging.h>
#include <rte_telemetry.h>
+#include "mempool_trace.h"
#include "rte_mempool.h"
-#include "rte_mempool_trace.h"
TAILQ_HEAD(rte_mempool_list, rte_tailq_entry);
@@ -11,7 +11,7 @@
#include <rte_errno.h>
#include <dev_driver.h>
-#include "rte_mempool_trace.h"
+#include "mempool_trace.h"
/* indirect jump table to support external memory pools. */
struct rte_mempool_ops_table rte_mempool_ops_table = {
@@ -47,22 +47,8 @@ EXPERIMENTAL {
__rte_mempool_trace_generic_get;
__rte_mempool_trace_get_bulk;
__rte_mempool_trace_get_contig_blocks;
- __rte_mempool_trace_create;
- __rte_mempool_trace_create_empty;
- __rte_mempool_trace_free;
- __rte_mempool_trace_populate_iova;
- __rte_mempool_trace_populate_virt;
- __rte_mempool_trace_populate_default;
- __rte_mempool_trace_populate_anon;
- __rte_mempool_trace_cache_create;
- __rte_mempool_trace_cache_free;
__rte_mempool_trace_default_cache;
- __rte_mempool_trace_get_page_size;
__rte_mempool_trace_cache_flush;
- __rte_mempool_trace_ops_populate;
- __rte_mempool_trace_ops_alloc;
- __rte_mempool_trace_ops_free;
- __rte_mempool_trace_set_ops_byname;
};
INTERNAL {