DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier MATZ <olivier.matz@6wind.com>
To: Zoltan Kiss <zoltan.kiss@linaro.org>, dev@dpdk.org,  dev@openvswitch.org
Subject: Re: [dpdk-dev] ovs-dpdk: placing the metadata
Date: Thu, 26 Mar 2015 09:44:51 +0100
Message-ID: <5513C703.6040908@6wind.com> (raw)
In-Reply-To: <55130506.4090000@linaro.org>

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

      reply	other threads:[~2015-03-26  8:44 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-24 17:42 Zoltan Kiss
2015-03-25 17:04 ` Olivier MATZ
2015-03-25 18:57   ` Zoltan Kiss
2015-03-26  8:44     ` Olivier MATZ [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5513C703.6040908@6wind.com \
    --to=olivier.matz@6wind.com \
    --cc=dev@dpdk.org \
    --cc=dev@openvswitch.org \
    --cc=zoltan.kiss@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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