[dpdk-dev] difficulty w/ RTE_NEXT_ABI

[dpdk-dev] difficulty w/ RTE_NEXT_ABI

[Matthew Hall]

I was trying to rebase my DPDK onto v2.1.0 and I came across some very 
confusing code in examples/l3fwd/main.c .

So... this code used the RTE_NEXT_ABI macros on a change which does not appear 
to affect the API... on a function that is marked always_inline ???

Maybe I missed something but this seems pointless. An always_inline function 
is going to have to be recompiled in any case.

Now I have no clue what would makes sense for my version of the function 
either... because RTE_NEXT_ABI is a binary on/off but trying to track a 
multi-variate quantity of ABI updates.

For now I guess I have to write it like this inside the RTE_NEXT_ABI:

rfc1812_process(struct ipv4_hdr *ipv4_hdr, uint32_t *dp, uint32_t ptype)

This seems unpleasant and kind of painful. What did I miss here?

Matthew.

static inline __attribute__((always_inline)) void
<<<<<<< 8e29af8a2843b6342dbc72db43ac82c9d29695bf
#ifdef RTE_NEXT_ABI
rfc1812_process(struct ipv4_hdr *ipv4_hdr, uint16_t *dp, uint32_t ptype)
#else
rfc1812_process(struct ipv4_hdr *ipv4_hdr, uint16_t *dp, uint32_t flags)
#endif
=======
rfc1812_process(struct ipv4_hdr *ipv4_hdr, uint32_t *dp, uint32_t flags)
>>>>>>> examples: update examples to use 24 bit extended next hop
{

Re: [dpdk-dev] difficulty w/ RTE_NEXT_ABI

[Thomas Monjalon]

2015-11-21 03:49, Matthew Hall:
> I was trying to rebase my DPDK onto v2.1.0 and I came across some very 
> confusing code in examples/l3fwd/main.c .
> 
> So... this code used the RTE_NEXT_ABI macros on a change which does not appear 
> to affect the API... on a function that is marked always_inline ???
> 
> Maybe I missed something but this seems pointless. An always_inline function 
> is going to have to be recompiled in any case.
> 
> Now I have no clue what would makes sense for my version of the function 
> either... because RTE_NEXT_ABI is a binary on/off but trying to track a 
> multi-variate quantity of ABI updates.
> 
> For now I guess I have to write it like this inside the RTE_NEXT_ABI:
> 
> rfc1812_process(struct ipv4_hdr *ipv4_hdr, uint32_t *dp, uint32_t ptype)
> 
> This seems unpleasant and kind of painful. What did I miss here?
> 
> Matthew.
> 
> static inline __attribute__((always_inline)) void
> <<<<<<< 8e29af8a2843b6342dbc72db43ac82c9d29695bf
> #ifdef RTE_NEXT_ABI
> rfc1812_process(struct ipv4_hdr *ipv4_hdr, uint16_t *dp, uint32_t ptype)
> #else
> rfc1812_process(struct ipv4_hdr *ipv4_hdr, uint16_t *dp, uint32_t flags)
> #endif

The new mbuf provides packet type instead of flags.
So the processing in this function is changed and the variable name is
different to reflect this.

Re: [dpdk-dev] difficulty w/ RTE_NEXT_ABI

[Matthew Hall]

On Sat, Nov 21, 2015 at 11:44:20AM +0100, Thomas Monjalon wrote:
> The new mbuf provides packet type instead of flags.
> So the processing in this function is changed and the variable name is
> different to reflect this.

But the data type of the variable is the same, and this is an internal 
always_inline function.

So again I am confused what advantage we got from RTE_NEXT_ABI here, and how 
you have multiple copies of RTE_NEXT_ABI on a single symbol when it is a 
binary variable.

This doesn't really answer the bigger question about the reasoning.

Matthew.

Re: [dpdk-dev] difficulty w/ RTE_NEXT_ABI

[Thomas Monjalon]

2015-11-21 19:25, Matthew Hall:
> On Sat, Nov 21, 2015 at 11:44:20AM +0100, Thomas Monjalon wrote:
> > The new mbuf provides packet type instead of flags.
> > So the processing in this function is changed and the variable name is
> > different to reflect this.
> 
> But the data type of the variable is the same, and this is an internal 
> always_inline function.

It is an example application using a mbuf feature which changes depending of
CONFIG_RTE_NEXT_ABI. The ABI is in the mbuf library not in the app.
The header of the app function is changed only for the variable name because
the semantic is changed.

> So again I am confused what advantage we got from RTE_NEXT_ABI here, and how 
> you have multiple copies of RTE_NEXT_ABI on a single symbol when it is a 
> binary variable.

I don't understand what is not clear here.

> This doesn't really answer the bigger question about the reasoning.

Probably because you don't ask clearly your question.
Please check the code and your question again.
Maybe that this reading may help: doc/guides/contributing/versioning.rst

Re: [dpdk-dev] difficulty w/ RTE_NEXT_ABI

[Matthew Hall]

On Sun, Nov 22, 2015 at 09:59:30PM +0100, Thomas Monjalon wrote:
> > So again I am confused what advantage we got from RTE_NEXT_ABI here, and how 
> > you have multiple copies of RTE_NEXT_ABI on a single symbol when it is a 
> > binary variable.
> 
> I don't understand what is not clear here.

OK. Let me restate it.

I was starting from an assumption that the purpose of RTE_NEXT_ABI was to 
allow ABI changes.

In most projects I worked on, a renaming of a variable when the data type is 
unchanged does not count as an ABI change. So it seems like this is different 
from the usual definition.

Secondly, if one is making an ABI change, like I was, to some code which was 
already changed once using RTE_NEXT_ABI, which part of the code do you change?

Do you make a third copy different from the first two copies? If you make a 
third copy, but RTE_NEXT_ABI is binary (i.e. it has two values, on and off) 
then what labeling do you apply to the third copy?

If you don't make a third copy, I am assuming you edit the copy marked with 
RTE_NEXT_ABI. But then what happens to a downstream user who wants to have 
RTE_NEXT_ABI with the first ABI change, and not your second ABI change?

Can you see what I am trying to ask now?

Matthew.

Re: [dpdk-dev] difficulty w/ RTE_NEXT_ABI

[Thomas Monjalon]

2015-11-22 18:25, Matthew Hall:
> On Sun, Nov 22, 2015 at 09:59:30PM +0100, Thomas Monjalon wrote:
> > > So again I am confused what advantage we got from RTE_NEXT_ABI here, and how 
> > > you have multiple copies of RTE_NEXT_ABI on a single symbol when it is a 
> > > binary variable.
> > 
> > I don't understand what is not clear here.
> 
> OK. Let me restate it.
> 
> I was starting from an assumption that the purpose of RTE_NEXT_ABI was to 
> allow ABI changes.

Yes it is a preview. The next release will have only the new ABI without #ifdef.

> In most projects I worked on, a renaming of a variable when the data type is 
> unchanged does not count as an ABI change. So it seems like this is different 
> from the usual definition.

No, it is not an ABI change.
As I said previously, the change is in the mbuf ABI.
So in the example app, the 2 ABIs are used with #ifdef.

> Secondly, if one is making an ABI change, like I was, to some code which was 
> already changed once using RTE_NEXT_ABI, which part of the code do you change?

If your change is sent upstream, you must rely on the new ABI because the old one
will be removed when your change will be integrated.
If it is a local change, it depends on which ABI you want to use.

> Do you make a third copy different from the first two copies? If you make a 
> third copy, but RTE_NEXT_ABI is binary (i.e. it has two values, on and off) 
> then what labeling do you apply to the third copy?
> 
> If you don't make a third copy, I am assuming you edit the copy marked with 
> RTE_NEXT_ABI. But then what happens to a downstream user who wants to have 
> RTE_NEXT_ABI with the first ABI change, and not your second ABI change?

In each release, there is only 2 ABIs: current and next.
In 2.2, there is only 1 ABI.

> Can you see what I am trying to ask now?

Yes. Hope my detailed answer is enough.

Re: [dpdk-dev] difficulty w/ RTE_NEXT_ABI

[Matthew Hall]

On Mon, Nov 23, 2015 at 01:13:32AM +0100, Thomas Monjalon wrote:
> If your change is sent upstream, you must rely on the new ABI because the old one
> will be removed when your change will be integrated.
> If it is a local change, it depends on which ABI you want to use.

I submitted separately to Bruce & Co. It is for LPM field expansion.

My question what to do came up in the context of a rebase to master.

> Yes. Hope my detailed answer is enough.

Thanks. It is less confusing now.

Matthew.