[dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area

[dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area

[Dan Gora]

Add an inline accessor function to return the starting address of
the private data area in the supplied mbuf.

If the user did not allocate space for a private data area in the
mbuf's memory pool, then return NULL.

This allows applications to easily access the private data area
between the struct rte_mbuf and the data buffer in the specified mbuf
without creating private macros or accessor functions.

Signed-off-by: Dan Gora <dg@adax.com>
---
 lib/librte_mbuf/rte_mbuf.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 8e6b4d292..0c4f8f698 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -728,6 +728,25 @@ rte_mbuf_to_baddr(struct rte_mbuf *md)
 	return buffer_addr;
 }
 
+/**
+ * Return the starting address of the private data area embedded in
+ * the given mbuf.
+ *
+ * @param md
+ *   The pointer to the mbuf.
+ * @return
+ *   The starting address of the private data area or NULL if there
+ *   is no private data area.
+ */
+static inline void *
+rte_mbuf_to_priv(struct rte_mbuf *md)
+{
+	if (md->priv_size == 0)
+		return NULL;
+
+	return RTE_PTR_ADD(md, sizeof(struct rte_mbuf));
+}
+
 /**
  * Returns TRUE if given mbuf is cloned by mbuf indirection, or FALSE
  * otherwise.
-- 
2.17.0.582.gccdcbd54c4

Re: [dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area

[Burakov, Anatoly]

On 08-Jun-18 12:54 AM, Dan Gora wrote:
> Add an inline accessor function to return the starting address of
> the private data area in the supplied mbuf.
> 
> If the user did not allocate space for a private data area in the
> mbuf's memory pool, then return NULL.
> 
> This allows applications to easily access the private data area
> between the struct rte_mbuf and the data buffer in the specified mbuf
> without creating private macros or accessor functions.
> 
> Signed-off-by: Dan Gora <dg@adax.com>
> ---

<...>

> +static inline void *
> +rte_mbuf_to_priv(struct rte_mbuf *md)
> +{
> +	if (md->priv_size == 0)
> +		return NULL;
> +
> +	return RTE_PTR_ADD(md, sizeof(struct rte_mbuf));
> +}

Hi Dan,

New API's should be marked as __rte_experimental for at least one release.

> +
>   /**
>    * Returns TRUE if given mbuf is cloned by mbuf indirection, or FALSE
>    * otherwise.
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area

[Andrew Rybchenko]

On 06/08/2018 02:54 AM, Dan Gora wrote:
> Add an inline accessor function to return the starting address of
> the private data area in the supplied mbuf.
>
> If the user did not allocate space for a private data area in the
> mbuf's memory pool, then return NULL.
>
> This allows applications to easily access the private data area
> between the struct rte_mbuf and the data buffer in the specified mbuf
> without creating private macros or accessor functions.
>
> Signed-off-by: Dan Gora <dg@adax.com>
> ---
>   lib/librte_mbuf/rte_mbuf.h | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 8e6b4d292..0c4f8f698 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -728,6 +728,25 @@ rte_mbuf_to_baddr(struct rte_mbuf *md)
>   	return buffer_addr;
>   }
>   
> +/**
> + * Return the starting address of the private data area embedded in
> + * the given mbuf.
> + *
> + * @param md
> + *   The pointer to the mbuf.
> + * @return
> + *   The starting address of the private data area or NULL if there
> + *   is no private data area.
> + */
> +static inline void *
> +rte_mbuf_to_priv(struct rte_mbuf *md)

Just a nit...
As I understand 'md' here follows previous function which is
rte_mbuf_to_baddr() and works with direct mbuf - that's why
parameter is named 'md' (mbuf direct). The most of functions
in the header use just 'm' for any mbuf.

> +{
> +	if (md->priv_size == 0)
> +		return NULL;
> +
> +	return RTE_PTR_ADD(md, sizeof(struct rte_mbuf));

Also a nit...
I'd use sizeof(*md) (or sizeof(*m) in fact as described above) here.
At least previous functions do it in such way.

> +}
> +
>   /**
>    * Returns TRUE if given mbuf is cloned by mbuf indirection, or FALSE
>    * otherwise.

Re: [dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area

[Wiles, Keith]

> On Jun 8, 2018, at 2:06 AM, Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
> On 06/08/2018 02:54 AM, Dan Gora wrote:
>> Add an inline accessor function to return the starting address of
>> the private data area in the supplied mbuf.
>> 
>> If the user did not allocate space for a private data area in the
>> mbuf's memory pool, then return NULL.
>> 
>> This allows applications to easily access the private data area
>> between the struct rte_mbuf and the data buffer in the specified mbuf
>> without creating private macros or accessor functions.
>> 
>> Signed-off-by: Dan Gora <dg@adax.com>
>> ---
>>  lib/librte_mbuf/rte_mbuf.h | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>> 
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 8e6b4d292..0c4f8f698 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -728,6 +728,25 @@ rte_mbuf_to_baddr(struct rte_mbuf *md)
>>  	return buffer_addr;
>>  }
>>  +/**
>> + * Return the starting address of the private data area embedded in
>> + * the given mbuf.
>> + *
>> + * @param md
>> + *   The pointer to the mbuf.
>> + * @return
>> + *   The starting address of the private data area or NULL if there
>> + *   is no private data area.
>> + */
>> +static inline void *
>> +rte_mbuf_to_priv(struct rte_mbuf *md)
> 
> Just a nit...
> As I understand 'md' here follows previous function which is
> rte_mbuf_to_baddr() and works with direct mbuf - that's why
> parameter is named 'md' (mbuf direct). The most of functions
> in the header use just 'm' for any mbuf.
> 
>> +{
>> +	if (md->priv_size == 0)
>> +		return NULL;
>> +
>> +	return RTE_PTR_ADD(md, sizeof(struct rte_mbuf));
> 
> Also a nit...
> I'd use sizeof(*md) (or sizeof(*m) in fact as described above) here.
> At least previous functions do it in such way.

I believe the sizeof(struct rte_mbuf) is much more readable then sizeof(*m) it makes the reader have to look up what ‘m’ is defined to. I know this is a small function, but readability is still a good reason to not use sizeof(*m) IMO.

> 
>> +}
>> +
>>  /**
>>   * Returns TRUE if given mbuf is cloned by mbuf indirection, or FALSE
>>   * otherwise.

Regards,
Keith

Re: [dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area

[Dan Gora]

Hi All,

Thanks for the feedback.

>> Just a nit...
>> As I understand 'md' here follows previous function which is
>> rte_mbuf_to_baddr() and works with direct mbuf - that's why
>> parameter is named 'md' (mbuf direct). The most of functions
>> in the header use just 'm' for any mbuf.

Ok, I'll fix this for v2.

>>> +{
>>> +    if (md->priv_size == 0)
>>> +            return NULL;
>>> +
>>> +    return RTE_PTR_ADD(md, sizeof(struct rte_mbuf));
>>
>> Also a nit...
>> I'd use sizeof(*md) (or sizeof(*m) in fact as described above) here.
>> At least previous functions do it in such way.
>
> I believe the sizeof(struct rte_mbuf) is much more readable then sizeof(*m) it makes the reader have to look up what ‘m’ is defined to. I know this is a small function, but readability is still a good reason to not use sizeof(*m) IMO.

On one hand, using sizeof(*m) is useful in case the type of 'm' ever
changes, you don't have to remember to change the arguments to sizeof.
On the other hand, it does make it slightly harder to read, but not a
lot really.

For me, it's six one way, half a dozen the other.   I just cut-pasted
this from the ipsec-secgw code.  I'm kind of inclined to leave it
sizeof(struct rte_mbuf) just to leave it clear.

Any opinion on my question from the cover letter?

Specifically when should rte_mbuf_XXX be used vs rte_pktmbuf_XXX for
mbuf API functions?  Why is there this inconsistency there?  Are they
just historical names which couldn't get changed?

One more quick question:

When sending a v2 of a patch series, should I resend the whole bundle,
even if there are no changes in the other patches or just send a v2 of
the patch which actually contains changes from the v1 version?

thanks
dan

Re: [dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area

[Andrew Rybchenko]

Hi Dan,

On 06/09/2018 03:24 AM, Dan Gora wrote:
>>>> +{
>>>> +    if (md->priv_size == 0)
>>>> +            return NULL;
>>>> +
>>>> +    return RTE_PTR_ADD(md, sizeof(struct rte_mbuf));
>>> Also a nit...
>>> I'd use sizeof(*md) (or sizeof(*m) in fact as described above) here.
>>> At least previous functions do it in such way.
>> I believe the sizeof(struct rte_mbuf) is much more readable then sizeof(*m) it makes the reader have to look up what ‘m’ is defined to. I know this is a small function, but readability is still a good reason to not use sizeof(*m) IMO.
> On one hand, using sizeof(*m) is useful in case the type of 'm' ever
> changes, you don't have to remember to change the arguments to sizeof.
> On the other hand, it does make it slightly harder to read, but not a
> lot really.
>
> For me, it's six one way, half a dozen the other.   I just cut-pasted
> this from the ipsec-secgw code.  I'm kind of inclined to leave it
> sizeof(struct rte_mbuf) just to leave it clear.

OK, I agree.

> Any opinion on my question from the cover letter?

Sorry, I was going to reply as I understand it, but forgot.

> Specifically when should rte_mbuf_XXX be used vs rte_pktmbuf_XXX for
> mbuf API functions?  Why is there this inconsistency there?  Are they
> just historical names which couldn't get changed?

I think that Olivier is best placed to answer it.
As I understand it is mainly historical right now, since ctrlmbuf API was
removed recently. For me, there is still a flavour of packet head in 
pktmbuf,
but boundaries are so vague.

> One more quick question:
>
> When sending a v2 of a patch series, should I resend the whole bundle,
> even if there are no changes in the other patches or just send a v2 of
> the patch which actually contains changes from the v1 version?

All patches should be resent in v2.

BTW, thinking about function I found out there is a trap in private area
size related to the function. I think that the function description should
highlight that rte_pktmbuf_priv_size(m->pool) should be used to
find out the size of private area since indirect mbuf has size of the
direct private are in its priv_size (but we return pointer to the indirect
mbuf (the mbuf itself) private area here).

Andrew.

Re: [dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area

[Dan Gora]

Hi Andrew,

On Sat, Jun 9, 2018 at 2:23 AM, Andrew Rybchenko
<arybchenko@solarflare.com> wrote:

> BTW, thinking about function I found out there is a trap in private area
> size related to the function. I think that the function description should
> highlight that rte_pktmbuf_priv_size(m->pool) should be used to
> find out the size of private area since indirect mbuf has size of the
> direct private are in its priv_size (but we return pointer to the indirect
> mbuf (the mbuf itself) private area here).
>

hmm... I guess I didn't realize that.

I think that what I'm going to do is just remove the check for
m->priv_size == 0 entirely.  It seems like checking the priv_size is
just going to cause more problems down the road than it is worth.  The
whole point of this was just to have a simple API function to access
the private area, not to fully save the programmer from shooting
him/herself in the foot by accessing a private data area which doesn't
necessarily exist.

thanks,
dan