DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] ovs-dpdk: placing the metadata
@ 2015-03-24 17:42 Zoltan Kiss
  2015-03-25 17:04 ` Olivier MATZ
  0 siblings, 1 reply; 4+ messages in thread
From: Zoltan Kiss @ 2015-03-24 17:42 UTC (permalink / raw)
  To: dev, dev

Hi,

I've noticed in lib/netdev-dpdk.c that __rte_pktmbuf_init() stores the 
packet metadata right after "struct rte_mbuf", and before the buffer data:

     /* start of buffer is just after mbuf structure */
     m->buf_addr = (char *)m + sizeof(struct dp_packet);

(struct dp_packet has the rte_mbuf as first member if DPDK enabled) 	

However, lib/librte_mbuf/rte_mbuf.h seems to codify that the buffer 
should start right after the rte_mbuf:

/**
  * Given the buf_addr returns the pointer to corresponding mbuf.
  */
#define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)

/**
  * Given the pointer to mbuf returns an address where it's  buf_addr
  * should point to.
  */
#define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)

These macros are used for attaching/detaching mbuf's to each other. This 
is the way the code retrieves the direct buffer from an indirect one, 
and vica versa. I think if we want to keep the metadata feature (which I 
guess is quite important), we need to add a pointer to rte_mbuf, which 
helps the direct and indirect structs to find each other. Something like:

	struct rte_mbuf *attach;    /**< Points to the other buffer if this one
					 is (in)direct. Otherwise NULL.  */

What do you think?

Regards,

Zoltan Kiss

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] ovs-dpdk: placing the metadata
  2015-03-24 17:42 [dpdk-dev] ovs-dpdk: placing the metadata Zoltan Kiss
@ 2015-03-25 17:04 ` Olivier MATZ
  2015-03-25 18:57   ` Zoltan Kiss
  0 siblings, 1 reply; 4+ messages in thread
From: Olivier MATZ @ 2015-03-25 17:04 UTC (permalink / raw)
  To: Zoltan Kiss, dev, dev

Hi Zoltan,

On 03/24/2015 06:42 PM, Zoltan Kiss wrote:
> Hi,
>
> I've noticed in lib/netdev-dpdk.c that __rte_pktmbuf_init() stores the
> packet metadata right after "struct rte_mbuf", and before the buffer data:
>
>      /* start of buffer is just after mbuf structure */
>      m->buf_addr = (char *)m + sizeof(struct dp_packet);
>
> (struct dp_packet has the rte_mbuf as first member if DPDK enabled)
>
> However, lib/librte_mbuf/rte_mbuf.h seems to codify that the buffer
> should start right after the rte_mbuf:
>
> /**
>   * Given the buf_addr returns the pointer to corresponding mbuf.
>   */
> #define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
>
> /**
>   * Given the pointer to mbuf returns an address where it's  buf_addr
>   * should point to.
>   */
> #define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
>
> These macros are used for attaching/detaching mbuf's to each other. This
> is the way the code retrieves the direct buffer from an indirect one,
> and vica versa. I think if we want to keep the metadata feature (which I
> guess is quite important), we need to add a pointer to rte_mbuf, which
> helps the direct and indirect structs to find each other. Something like:
>
>      struct rte_mbuf *attach;    /**< Points to the other buffer if this
> one
>                       is (in)direct. Otherwise NULL.  */
>
> What do you think?

I've just sent a patch that should fix this issue.
http://dpdk.org/ml/archives/dev/2015-March/015722.html

Let me know if you have any comment on it.

Regards,
Olivier

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] ovs-dpdk: placing the metadata
  2015-03-25 17:04 ` Olivier MATZ
@ 2015-03-25 18:57   ` Zoltan Kiss
  2015-03-26  8:44     ` Olivier MATZ
  0 siblings, 1 reply; 4+ messages in thread
From: Zoltan Kiss @ 2015-03-25 18:57 UTC (permalink / raw)
  To: Olivier MATZ, dev, dev

Hi Olivier,

On 25/03/15 17:04, Olivier MATZ wrote:
> Hi Zoltan,
>
> On 03/24/2015 06:42 PM, Zoltan Kiss wrote:
>> Hi,
>>
>> I've noticed in lib/netdev-dpdk.c that __rte_pktmbuf_init() stores the
>> packet metadata right after "struct rte_mbuf", and before the buffer
>> data:
>>
>>      /* start of buffer is just after mbuf structure */
>>      m->buf_addr = (char *)m + sizeof(struct dp_packet);
>>
>> (struct dp_packet has the rte_mbuf as first member if DPDK enabled)
>>
>> However, lib/librte_mbuf/rte_mbuf.h seems to codify that the buffer
>> should start right after the rte_mbuf:
>>
>> /**
>>   * Given the buf_addr returns the pointer to corresponding mbuf.
>>   */
>> #define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
>>
>> /**
>>   * Given the pointer to mbuf returns an address where it's  buf_addr
>>   * should point to.
>>   */
>> #define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
>>
>> These macros are used for attaching/detaching mbuf's to each other. This
>> is the way the code retrieves the direct buffer from an indirect one,
>> and vica versa. I think if we want to keep the metadata feature (which I
>> guess is quite important), we need to add a pointer to rte_mbuf, which
>> helps the direct and indirect structs to find each other. Something like:
>>
>>      struct rte_mbuf *attach;    /**< Points to the other buffer if this
>> one
>>                       is (in)direct. Otherwise NULL.  */
>>
>> What do you think?
>
> I've just sent a patch that should fix this issue.
> http://dpdk.org/ml/archives/dev/2015-March/015722.html
>
> Let me know if you have any comment on it.

I have some comments for the first patch:

> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index c3fcb80..050f3ac 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
I've sent in a separate patch for this file, I think it's just easier to 
ditch the old copy-pasted code, see "[PATCH] examples/vhost: use library 
routines instead of local copies"

> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 17ba791..4ced6d3 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -268,7 +268,7 @@ struct rte_mbuf {
>  	uint16_t data_len;        /**< Amount of data in segment buffer. */
>  	uint32_t pkt_len;         /**< Total pkt len: sum of all segments. */
>  	uint16_t vlan_tci;        /**< VLAN Tag Control Identifier (CPU order) */
> -	uint16_t reserved;
> +	uint16_t priv_size;       /**< size of the application private data */
>  	union {
>  		uint32_t rss;     /**< RSS hash result if RSS enabled */
>  		struct {
> @@ -320,15 +320,38 @@ struct rte_mbuf {
>  } __rte_cache_aligned;
>
>  /**
> - * Given the buf_addr returns the pointer to corresponding mbuf.
> + * Return the mbuf owning the given data buffer address.
> + *
> + * @param mi
> + *   The pointer to the indirect mbuf.
> + * @param buffer_addr
> + *   The address of the data buffer of the direct mbuf.
You don't need this parameter, it's mi->buf_addr.

> @@ -744,9 +767,11 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  {
>  	const struct rte_mempool *mp = m->pool;
> -	void *buf = RTE_MBUF_TO_BADDR(m);
> +	void *buf = rte_mbuf_to_baddr(m);
>  	uint32_t buf_len = mp->elt_size - sizeof(*m);
I don't see any reason to keep buf and buf_len, just assign straight to 
m->buf_addr and *len.
Besides that, you need to deduct m->priv_size from buf_len.

> -	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m);
> +
> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m) +
> +		m->priv_size;
>
>  	m->buf_addr = buf;
>  	m->buf_len = (uint16_t)buf_len;

The rest of the series looks good,

Reviewed-by: Zoltan Kiss <zoltan.kiss@linaro.org>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] ovs-dpdk: placing the metadata
  2015-03-25 18:57   ` Zoltan Kiss
@ 2015-03-26  8:44     ` Olivier MATZ
  0 siblings, 0 replies; 4+ messages in thread
From: Olivier MATZ @ 2015-03-26  8:44 UTC (permalink / raw)
  To: Zoltan Kiss, dev, dev

Hi Zoltan,

On 03/25/2015 07:57 PM, Zoltan Kiss wrote:
> I have some comments for the first patch:
>
>> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
>> index c3fcb80..050f3ac 100644
>> --- a/examples/vhost/main.c
>> +++ b/examples/vhost/main.c
> I've sent in a separate patch for this file, I think it's just easier to
> ditch the old copy-pasted code, see "[PATCH] examples/vhost: use library
> routines instead of local copies"

As Konstantin stated in the other thread, this modification is
wrong, sorry. I don't see why I changed this code instead of keeping
the initial logic. I'll fix that in the v2.

>>  /**
>> - * Given the buf_addr returns the pointer to corresponding mbuf.
>> + * Return the mbuf owning the given data buffer address.
>> + *
>> + * @param mi
>> + *   The pointer to the indirect mbuf.
>> + * @param buffer_addr
>> + *   The address of the data buffer of the direct mbuf.
> You don't need this parameter, it's mi->buf_addr.

That's correct. In this case, I propose to change the name of the
function in:
struct rte_mbuf *rte_mbuf_from_indirect(struct rte_mbuf *mi)

>> @@ -744,9 +767,11 @@ static inline void rte_pktmbuf_attach(struct
>> rte_mbuf *mi, struct rte_mbuf *md)
>>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>>  {
>>      const struct rte_mempool *mp = m->pool;
>> -    void *buf = RTE_MBUF_TO_BADDR(m);
>> +    void *buf = rte_mbuf_to_baddr(m);
>>      uint32_t buf_len = mp->elt_size - sizeof(*m);
> I don't see any reason to keep buf and buf_len, just assign straight to
> m->buf_addr and *len.
> Besides that, you need to deduct m->priv_size from buf_len.

Agree. I suggest something like:

static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
{
	const struct rte_mempool *mp = m->pool;
	void *buf = rte_mbuf_to_baddr(m);
	unsigned mhdr_size = (char *)buf - (char *)m;

	m->buf_addr = buf;
	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mhdr_size;
	m->buf_len = (uint16_t)(mp->elt_size - mhdr_size);
...


> The rest of the series looks good,
>
> Reviewed-by: Zoltan Kiss <zoltan.kiss@linaro.org>

Thank you for your review. I'll send a v2 today.

Regards,
Olivier

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-03-26  8:44 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-24 17:42 [dpdk-dev] ovs-dpdk: placing the metadata Zoltan Kiss
2015-03-25 17:04 ` Olivier MATZ
2015-03-25 18:57   ` Zoltan Kiss
2015-03-26  8:44     ` Olivier MATZ

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git