From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f178.google.com (mail-wi0-f178.google.com [209.85.212.178]) by dpdk.org (Postfix) with ESMTP id 1BE959AC1 for ; Wed, 25 Mar 2015 19:57:12 +0100 (CET) Received: by wixw10 with SMTP id w10so52104128wix.0 for ; Wed, 25 Mar 2015 11:57:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=8vDwStd3TKzyOPtOk5Y0ntZh12+uh0788Xwtn8pID0Q=; b=VA8zFGYaVg4Tkg7lEdQvuGaDBDsW/ICqPgDsHy8FJIUSSui/qtEIjFOI0VeGpSe+rB KVsvK1oVnbp81JIc+gnkDanl21l+pqpEJO7nmbNfISdG51jpHEF6/ypFm4aR42S+q8ZK rZsPIp+sm58FNEdMuqxmxi+CGqQ6JcGxTFxZND50FLhpEupNCeWQ20jayd/+bR7QK0pf RYB7HUD37fHaQC4aWo3cZxW6xS9ebZQZSZY7BuI2R+sfOT66grUYTXXqn5kZSSAMW4EI J3SsYIuhBe8FTPztVjhTm0v65woMcbaP5U1E2sEUlnmSwTp2F5R9U2eQ+H+GFAU8h51d 9NQw== X-Gm-Message-State: ALoCoQm5iJs5stiDw5UCbXovCnutHGOjEhxW5pamUyPyM/0rp62vjJBSxn3+inTRA/uMFgsZQMC0 X-Received: by 10.180.98.134 with SMTP id ei6mr11978511wib.85.1427309831960; Wed, 25 Mar 2015 11:57:11 -0700 (PDT) Received: from [192.168.0.101] ([90.152.119.35]) by mx.google.com with ESMTPSA id hj10sm4918719wjc.48.2015.03.25.11.57.10 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 25 Mar 2015 11:57:10 -0700 (PDT) Message-ID: <55130506.4090000@linaro.org> Date: Wed, 25 Mar 2015 18:57:10 +0000 From: Zoltan Kiss User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Olivier MATZ , dev@dpdk.org, dev@openvswitch.org References: <5511A1F0.40605@linaro.org> <5512EA87.1020707@6wind.com> In-Reply-To: <5512EA87.1020707@6wind.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailman-Approved-At: Wed, 25 Mar 2015 23:52:36 +0100 Subject: Re: [dpdk-dev] ovs-dpdk: placing the metadata X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Mar 2015 18:57:12 -0000 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