[dpdk-dev] Marking symbols as experimental in the headers only

[dpdk-dev] Marking symbols as experimental in the headers only

[David Marchand]

Hello Neil,

Looking at
http://doc.dpdk.org/guides/contributing/versioning.html#experimental-apis,
is there a real need to mark both the definition and the declaration of a
symbol with the __rte_experimental marker ?

My understanding is that the whole point of having this marker is so that
rte_compat.h check trigger warnings when trying to use such a symbol from
the caller side.
As long as the header where the symbol is published is included from the
file that defines the symbol, then the forward declaration ensures the
symbol will catch the tag, right ?

I would prefer marking the symbols only once in the header and write this
as the recommended way in the documentation.

Do you see any issue doing this ?


We have found an inconsistency, with a symbol marked as experimental in its
.c but not the .h ... will come up with a fix later.


-- 
David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[Ferruh Yigit]

On 12/3/2018 1:01 PM, David Marchand wrote:
> Hello Neil,
> 
> Looking at
> http://doc.dpdk.org/guides/contributing/versioning.html#experimental-apis,
> is there a real need to mark both the definition and the declaration of a
> symbol with the __rte_experimental marker ?
> 
> My understanding is that the whole point of having this marker is so that
> rte_compat.h check trigger warnings when trying to use such a symbol from
> the caller side.
> As long as the header where the symbol is published is included from the
> file that defines the symbol, then the forward declaration ensures the
> symbol will catch the tag, right ?
> 
> I would prefer marking the symbols only once in the header and write this
> as the recommended way in the documentation.

+1

> 
> Do you see any issue doing this ?
> 
> 
> We have found an inconsistency, with a symbol marked as experimental in its
> .c but not the .h ... will come up with a fix later.
> 
> 

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[Neil Horman]

On Mon, Dec 03, 2018 at 02:01:02PM +0100, David Marchand wrote:
> Hello Neil,
> 
> Looking at
> http://doc.dpdk.org/guides/contributing/versioning.html#experimental-apis,
> is there a real need to mark both the definition and the declaration of a
> symbol with the __rte_experimental marker ?
> 
When I initially wrote the feature I marked both the function prototype and its
definition with the macro to be consistent, as I like to make both declaration
and definition look visually simmilar, just to ease readability, but I'm not
bound to doing it that way per-se

As to weather or not you can only mark the declaration as experimental, I'm not
100% sure.  I think thats an artifact of the compiler.  That is to say, the
macro expands to a compiler attribute that is applied at compile time, and
checked at link time, and I'm not sure what clang or gcc will do if there is a
discrepancy between the attributes listed in the declaration and the definition.

I would say, give it a try, and if you can demonstrate that adding the attribute
only to the declaration results in a link-time warning when external code
attempts to call an experimental function, I would have no problem handling it
that way.

> My understanding is that the whole point of having this marker is so that
> rte_compat.h check trigger warnings when trying to use such a symbol from
> the caller side.
More specifically, its there so that when external code attempts to make a call
to the experimental function, the compiler warns the user about said usage.

> As long as the header where the symbol is published is included from the
> file that defines the symbol, then the forward declaration ensures the
> symbol will catch the tag, right ?
> 
Not 100% sure on that.  I say that because the attribute adds a note in the
object file of the function definition, and at link time its checked so that
calls to that function generate warnings.  What needs to happen is that the
deprecation note needs to make its way into the definitions object code.  If
that can be accomplished by just annotating the declaration, thats great, but I
would suspect that its more likely that the definition needs to have the tag
more than the declaration.  I've been wrong before though, so likely some
experimentation is called for here.

> I would prefer marking the symbols only once in the header and write this
> as the recommended way in the documentation.
> 
> Do you see any issue doing this ?
> 
I'm fine with it, as long as (as I note above), you can demonstrate that
deprecation warnings are still issued to experimental api users if you only mark
the declaration as __rte_experimental


> 
> We have found an inconsistency, with a symbol marked as experimental in its
> .c but not the .h ... will come up with a fix later.
> 
> 
> -- 
> David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[David Marchand]

On Mon, Dec 3, 2018 at 5:48 PM Neil Horman <nhorman@tuxdriver.com> wrote:

> On Mon, Dec 03, 2018 at 02:01:02PM +0100, David Marchand wrote:
> > Hello Neil,
> >
> > Looking at
> >
> http://doc.dpdk.org/guides/contributing/versioning.html#experimental-apis,
> > is there a real need to mark both the definition and the declaration of a
> > symbol with the __rte_experimental marker ?
> >
> When I initially wrote the feature I marked both the function prototype
> and its
> definition with the macro to be consistent, as I like to make both
> declaration
> and definition look visually simmilar, just to ease readability, but I'm
> not
> bound to doing it that way per-se
>
> As to weather or not you can only mark the declaration as experimental,
> I'm not
> 100% sure.  I think thats an artifact of the compiler.  That is to say, the
> macro expands to a compiler attribute that is applied at compile time, and
> checked at link time, and I'm not sure what clang or gcc will do if there
> is a
> discrepancy between the attributes listed in the declaration and the
> definition.
>
> I would say, give it a try, and if you can demonstrate that adding the
> attribute
> only to the declaration results in a link-time warning when external code
> attempts to call an experimental function, I would have no problem
> handling it
> that way.
>

Did not experiment yet, putting this in my todolist.
Just, I can see that lib/librte_pipeline/ at least only marks the
declarations.


-- 
David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[Neil Horman]

On Tue, Dec 04, 2018 at 09:21:43AM +0100, David Marchand wrote:
> On Mon, Dec 3, 2018 at 5:48 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Mon, Dec 03, 2018 at 02:01:02PM +0100, David Marchand wrote:
> > > Hello Neil,
> > >
> > > Looking at
> > >
> > http://doc.dpdk.org/guides/contributing/versioning.html#experimental-apis,
> > > is there a real need to mark both the definition and the declaration of a
> > > symbol with the __rte_experimental marker ?
> > >
> > When I initially wrote the feature I marked both the function prototype
> > and its
> > definition with the macro to be consistent, as I like to make both
> > declaration
> > and definition look visually simmilar, just to ease readability, but I'm
> > not
> > bound to doing it that way per-se
> >
> > As to weather or not you can only mark the declaration as experimental,
> > I'm not
> > 100% sure.  I think thats an artifact of the compiler.  That is to say, the
> > macro expands to a compiler attribute that is applied at compile time, and
> > checked at link time, and I'm not sure what clang or gcc will do if there
> > is a
> > discrepancy between the attributes listed in the declaration and the
> > definition.
> >
> > I would say, give it a try, and if you can demonstrate that adding the
> > attribute
> > only to the declaration results in a link-time warning when external code
> > attempts to call an experimental function, I would have no problem
> > handling it
> > that way.
> >
> 
> Did not experiment yet, putting this in my todolist.
> Just, I can see that lib/librte_pipeline/ at least only marks the
> declarations.
> 
I just tried it. If I turn off ALLOW_EXPERIMENTAL_APIS, the ip_pipeline example
breaks with warnings about deprecated functions:

/home/nhorman/git/dpdk/examples/ip_pipeline/action.c:60:4: error: ‘rte_table_hash_crc_key8’ is deprecated: Symbol is not yet part of stable ABI [-Werror=deprecated-declarations]
    params->lb.f_hash = rte_table_hash_crc_key8;
    ^~~~~~
In file included from /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:10:
/home/nhorman/git/dpdk/x86_64-native-linuxapp-gcc/include/rte_table_hash_func.h:56:1: note: declared here
 rte_table_hash_crc_key8(void *key, void *mask, __rte_unused uint32_t key_size,
 ^~~~~~~~~~~~~~~~~~~~~~~
/home/nhorman/git/dpdk/examples/ip_pipeline/action.c:64:4: error: ‘rte_table_hash_crc_key16’ is deprecated: Symbol is not yet part of stable ABI [-Werror=deprecated-declarations]
    params->lb.f_hash = rte_table_hash_crc_key16;
    ^~~~~~
...

That is sufficient for me to conclude that __rte_experimental only needs to be
set on the declaration, not the definiton as well.

If you would like to make this adjustment, I'm fine with it, though be aware,
you will likely need to make some adjustments to the check-experimental-syms
script to account for this

Neil

> 
> -- 
> David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[David Marchand]

On Tue, Dec 4, 2018 at 4:16 PM Neil Horman <nhorman@tuxdriver.com> wrote:

> On Tue, Dec 04, 2018 at 09:21:43AM +0100, David Marchand wrote:
> > On Mon, Dec 3, 2018 at 5:48 PM Neil Horman <nhorman@tuxdriver.com>
> wrote:
> > > On Mon, Dec 03, 2018 at 02:01:02PM +0100, David Marchand wrote:
> > > I would say, give it a try, and if you can demonstrate that adding the
> > > attribute
> > > only to the declaration results in a link-time warning when external
> code
> > > attempts to call an experimental function, I would have no problem
> > > handling it
> > > that way.
> > >
> >
> > Did not experiment yet, putting this in my todolist.
> > Just, I can see that lib/librte_pipeline/ at least only marks the
> > declarations.
> >
> I just tried it. If I turn off ALLOW_EXPERIMENTAL_APIS, the ip_pipeline
> example
> breaks with warnings about deprecated functions:
>
> /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:60:4: error:
> ‘rte_table_hash_crc_key8’ is deprecated: Symbol is not yet part of stable
> ABI [-Werror=deprecated-declarations]
>     params->lb.f_hash = rte_table_hash_crc_key8;
>     ^~~~~~
> In file included from
> /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:10:
> /home/nhorman/git/dpdk/x86_64-native-linuxapp-gcc/include/rte_table_hash_func.h:56:1:
> note: declared here
>  rte_table_hash_crc_key8(void *key, void *mask, __rte_unused uint32_t
> key_size,
>  ^~~~~~~~~~~~~~~~~~~~~~~
> /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:64:4: error:
> ‘rte_table_hash_crc_key16’ is deprecated: Symbol is not yet part of stable
> ABI [-Werror=deprecated-declarations]
>     params->lb.f_hash = rte_table_hash_crc_key16;
>     ^~~~~~
> ...
>
> That is sufficient for me to conclude that __rte_experimental only needs
> to be
> set on the declaration, not the definiton as well.
>

Thanks for the test.
I tried with clang and this works fine as well.
Ferruh, could you do a little test with icc?


> If you would like to make this adjustment, I'm fine with it, though be
> aware,
> you will likely need to make some adjustments to the
> check-experimental-syms
> script to account for this
>

I find it much easier to track and, while doing the patch, I have found
another issue in the bbdev library header (another patch coming).
Under the impression that this can go undetected for quite some while...
If no one complains, yes, I am for this adjustment.


I am not sure I see what you mean on check-experimental-syms.sh.
I would only do a s/definition/declaration/ in the error message.
Do you have something else in mind ?


Btw, looking at the documentation, I can find no mention about meson and a
quick grep on check-experimental-syms.sh only catches mk/internal/
rte.compile-pre.mk.
Is there a trick ? or is meson simply not __rte_experimental aware ?


-- 
David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Marchand
> Sent: Tuesday, December 4, 2018 12:48 PM
> To: nhorman@tuxdriver.com; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Timothy Redaelli <tredaelli@redhat.com>;
> adrien.mazarguil@6wind.com
> Subject: Re: [dpdk-dev] Marking symbols as experimental in the headers
> only
> 
> On Tue, Dec 4, 2018 at 4:16 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Tue, Dec 04, 2018 at 09:21:43AM +0100, David Marchand wrote:
> > > On Mon, Dec 3, 2018 at 5:48 PM Neil Horman <nhorman@tuxdriver.com>
> > wrote:
> > > > On Mon, Dec 03, 2018 at 02:01:02PM +0100, David Marchand wrote:
> > > > I would say, give it a try, and if you can demonstrate that adding
> > > > the attribute only to the declaration results in a link-time
> > > > warning when external
> > code
> > > > attempts to call an experimental function, I would have no problem
> > > > handling it that way.
> > > >
> > >
> > > Did not experiment yet, putting this in my todolist.
> > > Just, I can see that lib/librte_pipeline/ at least only marks the
> > > declarations.
> > >
> > I just tried it. If I turn off ALLOW_EXPERIMENTAL_APIS, the
> > ip_pipeline example breaks with warnings about deprecated functions:
> >
> > /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:60:4: error:
> > ‘rte_table_hash_crc_key8’ is deprecated: Symbol is not yet part of
> > stable ABI [-Werror=deprecated-declarations]
> >     params->lb.f_hash = rte_table_hash_crc_key8;
> >     ^~~~~~
> > In file included from
> > /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:10:
> > /home/nhorman/git/dpdk/x86_64-native-linuxapp-
> gcc/include/rte_table_hash_func.h:56:1:
> > note: declared here
> >  rte_table_hash_crc_key8(void *key, void *mask, __rte_unused uint32_t
> > key_size,  ^~~~~~~~~~~~~~~~~~~~~~~
> > /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:64:4: error:
> > ‘rte_table_hash_crc_key16’ is deprecated: Symbol is not yet part of
> > stable ABI [-Werror=deprecated-declarations]
> >     params->lb.f_hash = rte_table_hash_crc_key16;
> >     ^~~~~~
> > ...
> >
> > That is sufficient for me to conclude that __rte_experimental only
> > needs to be set on the declaration, not the definiton as well.
> >
> 
> Thanks for the test.
> I tried with clang and this works fine as well.
> Ferruh, could you do a little test with icc?
> 
> 
> > If you would like to make this adjustment, I'm fine with it, though be
> > aware,
> > you will likely need to make some adjustments to the
> > check-experimental-syms
> > script to account for this
> >
> 
> I find it much easier to track and, while doing the patch, I have found
> another issue in the bbdev library header (another patch coming).
> Under the impression that this can go undetected for quite some while...
> If no one complains, yes, I am for this adjustment.
> 
> 
> I am not sure I see what you mean on check-experimental-syms.sh.
> I would only do a s/definition/declaration/ in the error message.
> Do you have something else in mind ?
> 
> 
> Btw, looking at the documentation, I can find no mention about meson and a
> quick grep on check-experimental-syms.sh only catches mk/internal/
> rte.compile-pre.mk.
> Is there a trick ? or is meson simply not __rte_experimental aware ?
> 

Good point, I suspect it isn't. I never thought to investigate how to integrate that tracking into meson builds.

/Bruce

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[Neil Horman]

On Tue, Dec 04, 2018 at 09:48:22PM +0100, David Marchand wrote:
> On Tue, Dec 4, 2018 at 4:16 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Tue, Dec 04, 2018 at 09:21:43AM +0100, David Marchand wrote:
> > > On Mon, Dec 3, 2018 at 5:48 PM Neil Horman <nhorman@tuxdriver.com>
> > wrote:
> > > > On Mon, Dec 03, 2018 at 02:01:02PM +0100, David Marchand wrote:
> > > > I would say, give it a try, and if you can demonstrate that adding the
> > > > attribute
> > > > only to the declaration results in a link-time warning when external
> > code
> > > > attempts to call an experimental function, I would have no problem
> > > > handling it
> > > > that way.
> > > >
> > >
> > > Did not experiment yet, putting this in my todolist.
> > > Just, I can see that lib/librte_pipeline/ at least only marks the
> > > declarations.
> > >
> > I just tried it. If I turn off ALLOW_EXPERIMENTAL_APIS, the ip_pipeline
> > example
> > breaks with warnings about deprecated functions:
> >
> > /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:60:4: error:
> > ‘rte_table_hash_crc_key8’ is deprecated: Symbol is not yet part of stable
> > ABI [-Werror=deprecated-declarations]
> >     params->lb.f_hash = rte_table_hash_crc_key8;
> >     ^~~~~~
> > In file included from
> > /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:10:
> > /home/nhorman/git/dpdk/x86_64-native-linuxapp-gcc/include/rte_table_hash_func.h:56:1:
> > note: declared here
> >  rte_table_hash_crc_key8(void *key, void *mask, __rte_unused uint32_t
> > key_size,
> >  ^~~~~~~~~~~~~~~~~~~~~~~
> > /home/nhorman/git/dpdk/examples/ip_pipeline/action.c:64:4: error:
> > ‘rte_table_hash_crc_key16’ is deprecated: Symbol is not yet part of stable
> > ABI [-Werror=deprecated-declarations]
> >     params->lb.f_hash = rte_table_hash_crc_key16;
> >     ^~~~~~
> > ...
> >
> > That is sufficient for me to conclude that __rte_experimental only needs
> > to be
> > set on the declaration, not the definiton as well.
> >
> 
> Thanks for the test.
> I tried with clang and this works fine as well.
> Ferruh, could you do a little test with icc?
> 
> 
> > If you would like to make this adjustment, I'm fine with it, though be
> > aware,
> > you will likely need to make some adjustments to the
> > check-experimental-syms
> > script to account for this
> >
> 
> I find it much easier to track and, while doing the patch, I have found
> another issue in the bbdev library header (another patch coming).
> Under the impression that this can go undetected for quite some while...
> If no one complains, yes, I am for this adjustment.
> 
> 
> I am not sure I see what you mean on check-experimental-syms.sh.
> I would only do a s/definition/declaration/ in the error message.
> Do you have something else in mind ?
> 
> 
> Btw, looking at the documentation, I can find no mention about meson and a
> quick grep on check-experimental-syms.sh only catches mk/internal/
> rte.compile-pre.mk.
> Is there a trick ? or is meson simply not __rte_experimental aware ?
> 
All I was saying was that if you wanted to document the policy change, you might
need to check that script as its a reflection of that policy, and I couldn't
recall if it was grepping through .c and .h files (which might imply it needs to
change to reflect this policy).  I just looked however, and its checking object
files, so you should be ok.

Neil

> 
> -- 
> David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[David Marchand]

On Wed, Dec 5, 2018 at 1:23 PM Neil Horman <nhorman@tuxdriver.com> wrote:

> On Tue, Dec 04, 2018 at 09:48:22PM +0100, David Marchand wrote:
> > On Tue, Dec 4, 2018 at 4:16 PM Neil Horman <nhorman@tuxdriver.com>
> wrote:
> > > If you would like to make this adjustment, I'm fine with it, though be
> > > aware,
> > > you will likely need to make some adjustments to the
> > > check-experimental-syms
> > > script to account for this
> > >
> >
> > I am not sure I see what you mean on check-experimental-syms.sh.
> > I would only do a s/definition/declaration/ in the error message.
> > Do you have something else in mind ?
> All I was saying was that if you wanted to document the policy change, you
> might
> need to check that script as its a reflection of that policy, and I
> couldn't
> recall if it was grepping through .c and .h files (which might imply it
> needs to
> change to reflect this policy).  I just looked however, and its checking
> object
> files, so you should be ok.
>

Yes, thanks for the confirmation.

-- 
David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[David Marchand]

On Wed, Dec 5, 2018 at 2:22 PM David Marchand <david.marchand@redhat.com>
wrote:

>
> On Wed, Dec 5, 2018 at 1:23 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
>> On Tue, Dec 04, 2018 at 09:48:22PM +0100, David Marchand wrote:
>> > On Tue, Dec 4, 2018 at 4:16 PM Neil Horman <nhorman@tuxdriver.com>
>> wrote:
>> > > If you would like to make this adjustment, I'm fine with it, though be
>> > > aware,
>> > > you will likely need to make some adjustments to the
>> > > check-experimental-syms
>> > > script to account for this
>> > >
>> >
>> > I am not sure I see what you mean on check-experimental-syms.sh.
>> > I would only do a s/definition/declaration/ in the error message.
>> > Do you have something else in mind ?
>> All I was saying was that if you wanted to document the policy change,
>> you might
>> need to check that script as its a reflection of that policy, and I
>> couldn't
>> recall if it was grepping through .c and .h files (which might imply it
>> needs to
>> change to reflect this policy).  I just looked however, and its checking
>> object
>> files, so you should be ok.
>>
>
> Yes, thanks for the confirmation.
>

I have given it some more thought and did not send my patch that removes
all __rte_experimental from the definitions sites.
The real issue in the end is that the __rte_experimental in headers is the
most important thing and can be missed during reviews.
But I found no easy way to detect this.

Do you have any idea ?


-- 
David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[Neil Horman]

On Tue, Dec 18, 2018 at 11:41:34AM +0100, David Marchand wrote:
> On Wed, Dec 5, 2018 at 2:22 PM David Marchand <david.marchand@redhat.com>
> wrote:
> 
> >
> > On Wed, Dec 5, 2018 at 1:23 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> >> On Tue, Dec 04, 2018 at 09:48:22PM +0100, David Marchand wrote:
> >> > On Tue, Dec 4, 2018 at 4:16 PM Neil Horman <nhorman@tuxdriver.com>
> >> wrote:
> >> > > If you would like to make this adjustment, I'm fine with it, though be
> >> > > aware,
> >> > > you will likely need to make some adjustments to the
> >> > > check-experimental-syms
> >> > > script to account for this
> >> > >
> >> >
> >> > I am not sure I see what you mean on check-experimental-syms.sh.
> >> > I would only do a s/definition/declaration/ in the error message.
> >> > Do you have something else in mind ?
> >> All I was saying was that if you wanted to document the policy change,
> >> you might
> >> need to check that script as its a reflection of that policy, and I
> >> couldn't
> >> recall if it was grepping through .c and .h files (which might imply it
> >> needs to
> >> change to reflect this policy).  I just looked however, and its checking
> >> object
> >> files, so you should be ok.
> >>
> >
> > Yes, thanks for the confirmation.
> >
> 
> I have given it some more thought and did not send my patch that removes
> all __rte_experimental from the definitions sites.
> The real issue in the end is that the __rte_experimental in headers is the
> most important thing and can be missed during reviews.
> But I found no easy way to detect this.
> 
> Do you have any idea ?
> 
The most direct way is to add a regular expression search to the script that
checks the object files.  That would be some tricky grep/awk magic, but it
should be possible

Neil

> 
> -- 
> David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[David Marchand]

On Tue, Dec 18, 2018 at 1:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:

> On Tue, Dec 18, 2018 at 11:41:34AM +0100, David Marchand wrote:
>
> > The real issue in the end is that the __rte_experimental in headers is
> the
> > most important thing and can be missed during reviews.
> > But I found no easy way to detect this.
> >
> > Do you have any idea ?
> >
> The most direct way is to add a regular expression search to the script
> that
> checks the object files.  That would be some tricky grep/awk magic, but it
> should be possible
>

This is what I feared :-).
Ok, I will have a try.


-- 
David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[David Marchand]

On Tue, Dec 18, 2018 at 1:27 PM David Marchand <david.marchand@redhat.com>
wrote:

>
> On Tue, Dec 18, 2018 at 1:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
>> On Tue, Dec 18, 2018 at 11:41:34AM +0100, David Marchand wrote:
>>
>> > The real issue in the end is that the __rte_experimental in headers is
>> the
>> > most important thing and can be missed during reviews.
>> > But I found no easy way to detect this.
>> >
>> > Do you have any idea ?
>> >
>> The most direct way is to add a regular expression search to the script
>> that
>> checks the object files.  That would be some tricky grep/awk magic, but it
>> should be possible
>>
>
So, wrote something that I hooked in rte.lib.mk.
On a fresh master branch, I can see:

== Build lib/librte_bbdev
ERROR: rte_bbdev_dequeue_dec_ops is not marked as experimental in this
library headers
ERROR: rte_bbdev_dequeue_enc_ops is not marked as experimental in this
library headers
ERROR: rte_bbdev_devices is not marked as experimental in this library
headers
ERROR: rte_bbdev_enqueue_dec_ops is not marked as experimental in this
library headers
ERROR: rte_bbdev_enqueue_enc_ops is not marked as experimental in this
library headers
ERROR: rte_bbdev_op_pool_create is not marked as experimental in this
library headers
ERROR: rte_bbdev_op_type_str is not marked as experimental in this library
headers

== Build lib/librte_cryptodev
ERROR: rte_crypto_asym_op_strings is not marked as experimental in this
library headers
ERROR: rte_crypto_asym_xform_strings is not marked as experimental in this
library headers

== Build lib/librte_vhost
ERROR: rte_vhost_va_from_guest_pa is not marked as experimental in this
library headers

Those warnings seem valid, need to double check (bbdev is already known).

But my script still needs some work to make it lighter...
A fresh build went from:
real    3m25.823s
user    2m42.026s
sys    1m2.730s
to:
real    3m42.442s
user    2m56.733s
sys    1m5.565s

I think I'd rather adapt it to hook in checkpatches.sh.
Maintainers can then ignore it when the check is broken (my regexp skills
are lacking :)).


-- 
David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[Neil Horman]

On Wed, Dec 19, 2018 at 10:12:06AM +0100, David Marchand wrote:
> On Tue, Dec 18, 2018 at 1:27 PM David Marchand <david.marchand@redhat.com>
> wrote:
> 
> >
> > On Tue, Dec 18, 2018 at 1:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> >> On Tue, Dec 18, 2018 at 11:41:34AM +0100, David Marchand wrote:
> >>
> >> > The real issue in the end is that the __rte_experimental in headers is
> >> the
> >> > most important thing and can be missed during reviews.
> >> > But I found no easy way to detect this.
> >> >
> >> > Do you have any idea ?
> >> >
> >> The most direct way is to add a regular expression search to the script
> >> that
> >> checks the object files.  That would be some tricky grep/awk magic, but it
> >> should be possible
> >>
> >
> So, wrote something that I hooked in rte.lib.mk.
> On a fresh master branch, I can see:
> 
> == Build lib/librte_bbdev
> ERROR: rte_bbdev_dequeue_dec_ops is not marked as experimental in this
> library headers
> ERROR: rte_bbdev_dequeue_enc_ops is not marked as experimental in this
> library headers
> ERROR: rte_bbdev_devices is not marked as experimental in this library
> headers
> ERROR: rte_bbdev_enqueue_dec_ops is not marked as experimental in this
> library headers
> ERROR: rte_bbdev_enqueue_enc_ops is not marked as experimental in this
> library headers
> ERROR: rte_bbdev_op_pool_create is not marked as experimental in this
> library headers
> ERROR: rte_bbdev_op_type_str is not marked as experimental in this library
> headers
> 
> == Build lib/librte_cryptodev
> ERROR: rte_crypto_asym_op_strings is not marked as experimental in this
> library headers
> ERROR: rte_crypto_asym_xform_strings is not marked as experimental in this
> library headers
> 
> == Build lib/librte_vhost
> ERROR: rte_vhost_va_from_guest_pa is not marked as experimental in this
> library headers
> 
> Those warnings seem valid, need to double check (bbdev is already known).
> 
> But my script still needs some work to make it lighter...
> A fresh build went from:
> real    3m25.823s
> user    2m42.026s
> sys    1m2.730s
> to:
> real    3m42.442s
> user    2m56.733s
> sys    1m5.565s
> 
> I think I'd rather adapt it to hook in checkpatches.sh.
> Maintainers can then ignore it when the check is broken (my regexp skills
> are lacking :)).
> 
The fact that you got it to work this well, this quickly, says they're not that
bad to me :).

If you want to post a draft of your patch here, perhaps we can help find ways to
speed it up.

Well done.
Neil

> 
> -- 
> David Marchand

Re: [dpdk-dev] Marking symbols as experimental in the headers only

[David Marchand]

On Wed, Dec 19, 2018 at 1:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:

> On Wed, Dec 19, 2018 at 10:12:06AM +0100, David Marchand wrote:
> > On Tue, Dec 18, 2018 at 1:27 PM David Marchand <
> david.marchand@redhat.com>
> > wrote:
> > But my script still needs some work to make it lighter...
> > A fresh build went from:
> > real    3m25.823s
> > user    2m42.026s
> > sys    1m2.730s
> > to:
> > real    3m42.442s
> > user    2m56.733s
> > sys    1m5.565s
> >
> > I think I'd rather adapt it to hook in checkpatches.sh.
> > Maintainers can then ignore it when the check is broken (my regexp skills
> > are lacking :)).
> >
> The fact that you got it to work this well, this quickly, says they're not
> that
> bad to me :).
>
> If you want to post a draft of your patch here, perhaps we can help find
> ways to
> speed it up.
>

Here it is.
http://mails.dpdk.org/archives/dev/2018-December/121628.html

-- 
David Marchand