Message ID | 1476351445-18102-1-git-send-email-wei.dai@intel.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Thomas Monjalon |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id B343258EC; Thu, 13 Oct 2016 11:39:18 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id CD28258D6 for <dev@dpdk.org>; Thu, 13 Oct 2016 11:39:16 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP; 13 Oct 2016 02:39:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,339,1473145200"; d="scan'208";a="772109089" Received: from dpdk4.bj.intel.com ([172.16.182.178]) by FMSMGA003.fm.intel.com with ESMTP; 13 Oct 2016 02:39:15 -0700 From: Wei Dai <wei.dai@intel.com> To: dev@dpdk.org, sergio.gonzalez.monroy@intel.com, jianfeng.tan@intel.com, wei.dai@intel.com Date: Thu, 13 Oct 2016 17:37:25 +0800 Message-Id: <1476351445-18102-1-git-send-email-wei.dai@intel.com> X-Mailer: git-send-email 2.7.4 Subject: [dpdk-dev] [PATCH] mempool: fix search of maximum contiguous pages X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Wei Dai
Oct. 13, 2016, 9:37 a.m. UTC
paddr[i] + pg_sz always points to the start physical address of the
2nd page after pddr[i], so only up to 2 pages can be combinded to
be used. With this revision, more than 2 pages can be used.
Fixes: 84121f197187 ("mempool: store memory chunks in a list")
Signed-off-by: Wei Dai <wei.dai@intel.com>
---
lib/librte_mempool/rte_mempool.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Comments
+Olivier On 13/10/2016 10:37, Wei Dai wrote: > paddr[i] + pg_sz always points to the start physical address of the > 2nd page after pddr[i], so only up to 2 pages can be combinded to > be used. With this revision, more than 2 pages can be used. > > Fixes: 84121f197187 ("mempool: store memory chunks in a list") > > Signed-off-by: Wei Dai <wei.dai@intel.com> > --- > lib/librte_mempool/rte_mempool.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > index 71017e1..e3e254a 100644 > --- a/lib/librte_mempool/rte_mempool.c > +++ b/lib/librte_mempool/rte_mempool.c > @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct rte_mempool *mp, char *vaddr, > > for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) { > > + phys_addr_t paddr_next; > + paddr_next = paddr[i] + pg_sz; > + > /* populate with the largest group of contiguous pages */ > for (n = 1; (i + n) < pg_num && > - paddr[i] + pg_sz == paddr[i+n]; n++) > + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz) > ; > > ret = rte_mempool_populate_phys(mp, vaddr + i * pg_sz,
Hi > > Signed-off-by: Wei Dai <wei.dai@intel.com> > --- > lib/librte_mempool/rte_mempool.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > index 71017e1..e3e254a 100644 > --- a/lib/librte_mempool/rte_mempool.c > +++ b/lib/librte_mempool/rte_mempool.c > @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct rte_mempool *mp, char *vaddr, > > for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) { > > + phys_addr_t paddr_next; > + paddr_next = paddr[i] + pg_sz; > + > /* populate with the largest group of contiguous pages */ > for (n = 1; (i + n) < pg_num && > - paddr[i] + pg_sz == paddr[i+n]; n++) > + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz) > ; Good catch. Why not just paddr[i + n - 1] != paddr[i + n]? Then you don't need extra variable (paddr_next) here. Konstantin > > ret = rte_mempool_populate_phys(mp, vaddr + i * pg_sz, > -- > 2.7.4
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin > Sent: Thursday, October 13, 2016 10:54 AM > To: Dai, Wei <wei.dai@intel.com>; dev@dpdk.org; Gonzalez Monroy, Sergio <sergio.gonzalez.monroy@intel.com>; Tan, Jianfeng > <jianfeng.tan@intel.com>; Dai, Wei <wei.dai@intel.com> > Subject: Re: [dpdk-dev] [PATCH] mempool: fix search of maximum contiguous pages > > Hi > > > > > Signed-off-by: Wei Dai <wei.dai@intel.com> > > --- > > lib/librte_mempool/rte_mempool.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > > index 71017e1..e3e254a 100644 > > --- a/lib/librte_mempool/rte_mempool.c > > +++ b/lib/librte_mempool/rte_mempool.c > > @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct rte_mempool *mp, char *vaddr, > > > > for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) { > > > > + phys_addr_t paddr_next; > > + paddr_next = paddr[i] + pg_sz; > > + > > /* populate with the largest group of contiguous pages */ > > for (n = 1; (i + n) < pg_num && > > - paddr[i] + pg_sz == paddr[i+n]; n++) > > + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz) > > ; > > Good catch. > Why not just paddr[i + n - 1] != paddr[i + n]? Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course. > Then you don't need extra variable (paddr_next) here. > Konstantin > > > > > ret = rte_mempool_populate_phys(mp, vaddr + i * pg_sz, > > -- > > 2.7.4
> > > diff --git a/lib/librte_mempool/rte_mempool.c > > > b/lib/librte_mempool/rte_mempool.c > > > index 71017e1..e3e254a 100644 > > > --- a/lib/librte_mempool/rte_mempool.c > > > +++ b/lib/librte_mempool/rte_mempool.c > > > @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct > > > rte_mempool *mp, char *vaddr, > > > > > > for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) { > > > > > > + phys_addr_t paddr_next; > > > + paddr_next = paddr[i] + pg_sz; > > > + > > > /* populate with the largest group of contiguous pages */ > > > for (n = 1; (i + n) < pg_num && > > > - paddr[i] + pg_sz == paddr[i+n]; n++) > > > + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz) > > > ; > > > > Good catch. > > Why not just paddr[i + n - 1] != paddr[i + n]? > > Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course. > > > Then you don't need extra variable (paddr_next) here. > > Konstantin Thank you, Konstantin 'paddr[i + n - 1] + pg_sz = paddr[i + n]' also can fix it and have straight meaning. But I assume that my revision with paddr_next += pg_sz may have a bit better performance. By the way, paddr[i] + n * pg_sz = paddr[i + n] can also resolve it. /Wei > > > > > > > > ret = rte_mempool_populate_phys(mp, vaddr + i * pg_sz, > > > -- > > > 2.7.4
> > > > > diff --git a/lib/librte_mempool/rte_mempool.c > > > > b/lib/librte_mempool/rte_mempool.c > > > > index 71017e1..e3e254a 100644 > > > > --- a/lib/librte_mempool/rte_mempool.c > > > > +++ b/lib/librte_mempool/rte_mempool.c > > > > @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct > > > > rte_mempool *mp, char *vaddr, > > > > > > > > for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) { > > > > > > > > + phys_addr_t paddr_next; > > > > + paddr_next = paddr[i] + pg_sz; > > > > + > > > > /* populate with the largest group of contiguous pages */ > > > > for (n = 1; (i + n) < pg_num && > > > > - paddr[i] + pg_sz == paddr[i+n]; n++) > > > > + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz) > > > > ; > > > > > > Good catch. > > > Why not just paddr[i + n - 1] != paddr[i + n]? > > > > Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course. > > > > > Then you don't need extra variable (paddr_next) here. > > > Konstantin > > Thank you, Konstantin > 'paddr[i + n - 1] + pg_sz = paddr[i + n]' also can fix it and have straight meaning. > But I assume that my revision with paddr_next += pg_sz may have a bit better performance. I don't think there would be any real difference, again it is not performance critical code-path. > By the way, paddr[i] + n * pg_sz = paddr[i + n] can also resolve it. Yes, that's one seems even better for me - make things more clear. Konstantin > > /Wei > > > > > > > > > > > > ret = rte_mempool_populate_phys(mp, vaddr + i * pg_sz, > > > > -- > > > > 2.7.4
Hi Wei, On 10/13/2016 02:31 PM, Ananyev, Konstantin wrote: > >> >>>>> diff --git a/lib/librte_mempool/rte_mempool.c >>>>> b/lib/librte_mempool/rte_mempool.c >>>>> index 71017e1..e3e254a 100644 >>>>> --- a/lib/librte_mempool/rte_mempool.c >>>>> +++ b/lib/librte_mempool/rte_mempool.c >>>>> @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct >>>>> rte_mempool *mp, char *vaddr, >>>>> >>>>> for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) { >>>>> >>>>> + phys_addr_t paddr_next; >>>>> + paddr_next = paddr[i] + pg_sz; >>>>> + >>>>> /* populate with the largest group of contiguous pages */ >>>>> for (n = 1; (i + n) < pg_num && >>>>> - paddr[i] + pg_sz == paddr[i+n]; n++) >>>>> + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz) >>>>> ; >>>> >>>> Good catch. >>>> Why not just paddr[i + n - 1] != paddr[i + n]? >>> >>> Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course. >>> >>>> Then you don't need extra variable (paddr_next) here. >>>> Konstantin >> >> Thank you, Konstantin >> 'paddr[i + n - 1] + pg_sz = paddr[i + n]' also can fix it and have straight meaning. >> But I assume that my revision with paddr_next += pg_sz may have a bit better performance. > > I don't think there would be any real difference, again it is not performance critical code-path. > >> By the way, paddr[i] + n * pg_sz = paddr[i + n] can also resolve it. > > Yes, that's one seems even better for me - make things more clear. Thank you for fixing this. My vote would go for "addr[i + n - 1] + pg_sz == paddr[i + n]" If you feel "paddr[i] + n * pg_sz = paddr[i + n]" is clearer, I have no problem with it either. Regards, Olivier
2016-10-13 17:05, Olivier MATZ: > Hi Wei, > > On 10/13/2016 02:31 PM, Ananyev, Konstantin wrote: > > > >> > >>>>> diff --git a/lib/librte_mempool/rte_mempool.c > >>>>> b/lib/librte_mempool/rte_mempool.c > >>>>> index 71017e1..e3e254a 100644 > >>>>> --- a/lib/librte_mempool/rte_mempool.c > >>>>> +++ b/lib/librte_mempool/rte_mempool.c > >>>>> @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct > >>>>> rte_mempool *mp, char *vaddr, > >>>>> > >>>>> for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) { > >>>>> > >>>>> + phys_addr_t paddr_next; > >>>>> + paddr_next = paddr[i] + pg_sz; > >>>>> + > >>>>> /* populate with the largest group of contiguous pages */ > >>>>> for (n = 1; (i + n) < pg_num && > >>>>> - paddr[i] + pg_sz == paddr[i+n]; n++) > >>>>> + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz) > >>>>> ; > >>>> > >>>> Good catch. > >>>> Why not just paddr[i + n - 1] != paddr[i + n]? > >>> > >>> Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course. > >>> > >>>> Then you don't need extra variable (paddr_next) here. > >>>> Konstantin > >> > >> Thank you, Konstantin > >> 'paddr[i + n - 1] + pg_sz = paddr[i + n]' also can fix it and have straight meaning. > >> But I assume that my revision with paddr_next += pg_sz may have a bit better performance. > > > > I don't think there would be any real difference, again it is not performance critical code-path. > > > >> By the way, paddr[i] + n * pg_sz = paddr[i + n] can also resolve it. > > > > Yes, that's one seems even better for me - make things more clear. > > Thank you for fixing this. > > My vote would go for "addr[i + n - 1] + pg_sz == paddr[i + n]" > > If you feel "paddr[i] + n * pg_sz = paddr[i + n]" is clearer, I have no > problem with it either. No answer from Wei Dai. Please Olivier advise what to do with this patch. Thanks
Hi Thomas, On 10/25/2016 04:37 PM, Thomas Monjalon wrote: > 2016-10-13 17:05, Olivier MATZ: >> Hi Wei, >> >> On 10/13/2016 02:31 PM, Ananyev, Konstantin wrote: >>> >>>> >>>>>>> diff --git a/lib/librte_mempool/rte_mempool.c >>>>>>> b/lib/librte_mempool/rte_mempool.c >>>>>>> index 71017e1..e3e254a 100644 >>>>>>> --- a/lib/librte_mempool/rte_mempool.c >>>>>>> +++ b/lib/librte_mempool/rte_mempool.c >>>>>>> @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct >>>>>>> rte_mempool *mp, char *vaddr, >>>>>>> >>>>>>> for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) { >>>>>>> >>>>>>> + phys_addr_t paddr_next; >>>>>>> + paddr_next = paddr[i] + pg_sz; >>>>>>> + >>>>>>> /* populate with the largest group of contiguous pages */ >>>>>>> for (n = 1; (i + n) < pg_num && >>>>>>> - paddr[i] + pg_sz == paddr[i+n]; n++) >>>>>>> + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz) >>>>>>> ; >>>>>> >>>>>> Good catch. >>>>>> Why not just paddr[i + n - 1] != paddr[i + n]? >>>>> >>>>> Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course. >>>>> >>>>>> Then you don't need extra variable (paddr_next) here. >>>>>> Konstantin >>>> >>>> Thank you, Konstantin >>>> 'paddr[i + n - 1] + pg_sz = paddr[i + n]' also can fix it and have straight meaning. >>>> But I assume that my revision with paddr_next += pg_sz may have a bit better performance. >>> >>> I don't think there would be any real difference, again it is not performance critical code-path. >>> >>>> By the way, paddr[i] + n * pg_sz = paddr[i + n] can also resolve it. >>> >>> Yes, that's one seems even better for me - make things more clear. >> >> Thank you for fixing this. >> >> My vote would go for "addr[i + n - 1] + pg_sz == paddr[i + n]" >> >> If you feel "paddr[i] + n * pg_sz = paddr[i + n]" is clearer, I have no >> problem with it either. > > No answer from Wei Dai. > Please Olivier advise what to do with this patch. > Thanks > I think it's good to have this fix in 16.11. I'm sending a v2 based on Wei's patch. Olivier
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c index 71017e1..e3e254a 100644 --- a/lib/librte_mempool/rte_mempool.c +++ b/lib/librte_mempool/rte_mempool.c @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct rte_mempool *mp, char *vaddr, for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) { + phys_addr_t paddr_next; + paddr_next = paddr[i] + pg_sz; + /* populate with the largest group of contiguous pages */ for (n = 1; (i + n) < pg_num && - paddr[i] + pg_sz == paddr[i+n]; n++) + paddr_next == paddr[i+n]; n++, paddr_next += pg_sz) ; ret = rte_mempool_populate_phys(mp, vaddr + i * pg_sz,