From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail6.bemta8.messagelabs.com (mail6.bemta8.messagelabs.com [216.82.243.55]) by kanga.kvack.org (Postfix) with ESMTP id 41CDF9000C6 for ; Mon, 3 Oct 2011 09:30:42 -0400 (EDT) Received: by vcbfo14 with SMTP id fo14so3803957vcb.14 for ; Mon, 03 Oct 2011 06:30:06 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20111003192458.14d198a3.kamezawa.hiroyu@jp.fujitsu.com> References: <20111003192458.14d198a3.kamezawa.hiroyu@jp.fujitsu.com> Date: Mon, 3 Oct 2011 21:30:06 +0800 Message-ID: Subject: Re: One comment on the __release_region in kernel/resource.c From: Wei Yang Content-Type: multipart/alternative; boundary=bcaec519638f1b424704ae64f944 Sender: owner-linux-mm@kvack.org List-ID: To: KAMEZAWA Hiroyuki Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org --bcaec519638f1b424704ae64f944 Content-Type: text/plain; charset=ISO-8859-1 2011/10/3 KAMEZAWA Hiroyuki > On Sun, 2 Oct 2011 21:57:07 +0800 > Wei Yang wrote: > > > Dear experts, > > > > I am viewing the source code of __release_region() in kernel/resource.c. > > And I have one comment for the performance issue. > > > > For example, we have a resource tree like this. > > 10-89 > > 20-79 > > 30-49 > > 55-59 > > 60-64 > > 65-69 > > 80-89 > > 100-279 > > > > If the caller wants to release a region of [50,59], the original code > will > > execute four times in the for loop in the subtree of 20-79. > > > > After changing the code below, it will execute two times instead. > > > > By using the "git annotate", I see this code is committed by Linus as the > > initial version. So don't get more information about why this code is > > written > > in this way. > > > > Maybe the case I thought will not happen in the real world? > > > > Your comment is warmly welcome. :) > > > > diff --git a/kernel/resource.c b/kernel/resource.c > > index 8461aea..81525b4 100644 > > --- a/kernel/resource.c > > +++ b/kernel/resource.c > > @@ -931,7 +931,7 @@ void __release_region(struct resource *parent, > > resource_size_t start, > > for (;;) { > > struct resource *res = *p; > > > > - if (!res) > > + if (!res || res->start > start) > > Hmm ? > res->start > end ? > > I think res->start > start is fine. __release_region will release the exact the region, no overlap. So if res->start > start, this means there is no exact region to release. The required to release region doesn't exist. > Thanks, > -Kame > > -- Wei Yang Help You, Help Me --bcaec519638f1b424704ae64f944 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

2011/10/3 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
On Sun, 2 Oct 2011 21:57:07 +0800
Wei Yang <= weiyang.kernel@gmail.com> wrote:

> Dear experts,
>
> I am viewing the source code of __release_region() in kernel/resource.= c.
> And I have one comment for the performance issue.
>
> For example, we have a resource tree like this.
> 10-89
> =A0 =A020-79
> =A0 =A0 =A0 =A030-49
> =A0 =A0 =A0 =A055-59
> =A0 =A0 =A0 =A060-64
> =A0 =A0 =A0 =A065-69
> =A0 =A080-89
> 100-279
>
> If the caller wants to release a region of [50,59], the original code = will
> execute four times in the for loop in the subtree of 20-79.
>
> After changing the code below, it will execute two times instead.
>
> By using the "git annotate", I see this code is committed by= Linus as the
> initial version. So don't get more information about why this code= is
> written
> in this way.
>
> Maybe the case I thought will not happen in the real world?
>
> Your comment is warmly welcome. :)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 8461aea..81525b4 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -931,7 +931,7 @@ void __release_region(struct resource *parent,
> resource_size_t start,
> =A0 =A0 =A0 =A0for (;;) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct resource *res =3D *p;
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!res)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!res || res->start > start)
Hmm ?
=A0 =A0 =A0 =A0res->start > end ?

=A0I think res->start > start is fine.
__re= lease_region will release the exact the region, no overlap.
So if res-&g= t;start > start, this means there is no exact region to release.
The = required to release region doesn't exist.


Thanks,
-Kame




--
Wei Yang
Help You, H= elp Me

--bcaec519638f1b424704ae64f944-- -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: email@kvack.org