One comment on the __release_region in kernel/resource.c

One comment on the __release_region in kernel/resource.c

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

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)
                       break;
               if (res->start <= start && res->end >= end) {
                       if (!(res->flags & IORESOURCE_BUSY)) {

Wei Yang
Help You, Help Me

[-- Attachment #2: Type: text/html, Size: 1609 bytes --]

Re: One comment on the __release_region in kernel/resource.c

[KAMEZAWA Hiroyuki]

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
>    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 ?


Thanks,
-Kame

--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: One comment on the __release_region in kernel/resource.c

[Geert Uytterhoeven]

On Mon, Oct 3, 2011 at 12:24, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 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
>>    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
                                              ^^^^^^^
Do you really mean [50,59]?
I don't think that's allowed, as the tree has [55,59], so you would release a
larger region that allocated.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: One comment on the __release_region in kernel/resource.c

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

2011/10/3 Geert Uytterhoeven <geert@linux-m68k.org>

> On Mon, Oct 3, 2011 at 12:24, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > 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
> >>    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
>                                               ^^^^^^^
> Do you really mean [50,59]?
>
Yes.

> I don't think that's allowed, as the tree has [55,59], so you would release
> a
> larger region that allocated.
>
So you mean the case I mentioned will not happen?
Actually, I believe every developer should pass the resource region which
has been allocated.
While if some one made a mistake and pass a region which is not allocated
before and overlap
some "BUSY" region?


>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker.
> But
> when I'm talking to journalists I just say "programmer" or something like
> that.
>                                 -- Linus Torvalds
>



-- 
Wei Yang
Help You, Help Me

[-- Attachment #2: Type: text/html, Size: 2592 bytes --]

Re: One comment on the __release_region in kernel/resource.c

[Geert Uytterhoeven]

On Mon, Oct 3, 2011 at 15:35, Wei Yang <weiyang.kernel@gmail.com> wrote:
> 2011/10/3 Geert Uytterhoeven <geert@linux-m68k.org>
>> On Mon, Oct 3, 2011 at 12:24, KAMEZAWA Hiroyuki
>> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>> > 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
>> >>    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
>>                                              ^^^^^^^
>> Do you really mean [50,59]?
>
> Yes.
>>
>> I don't think that's allowed, as the tree has [55,59], so you would
>> release a
>> larger region that allocated.
>
> So you mean the case I mentioned will not happen?

Indeed, it should not happen.
Actually I'm surprised it doesn't return an error code.

> Actually, I believe every developer should pass the resource region which
> has been allocated.
> While if some one made a mistake and pass a region which is not allocated
> before and overlap
> some "BUSY" region?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: One comment on the __release_region in kernel/resource.c

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1949 bytes --]

2011/10/3 Geert Uytterhoeven <geert@linux-m68k.org>

> On Mon, Oct 3, 2011 at 15:35, Wei Yang <weiyang.kernel@gmail.com> wrote:
> > 2011/10/3 Geert Uytterhoeven <geert@linux-m68k.org>
> >> On Mon, Oct 3, 2011 at 12:24, KAMEZAWA Hiroyuki
> >> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >> > 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
> >> >>    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
> >>                                              ^^^^^^^
> >> Do you really mean [50,59]?
> >
> > Yes.
> >>
> >> I don't think that's allowed, as the tree has [55,59], so you would
> >> release a
> >> larger region that allocated.
> >
> > So you mean the case I mentioned will not happen?
>
> Indeed, it should not happen.
> Actually I'm surprised it doesn't return an error code.
>
Yes, it shouldn't happen.
But it need to handle the error case.

>
> > Actually, I believe every developer should pass the resource region which
> > has been allocated.
> > While if some one made a mistake and pass a region which is not allocated
> > before and overlap
> > some "BUSY" region?
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker.
> But
> when I'm talking to journalists I just say "programmer" or something like
> that.
>                                 -- Linus Torvalds
>



-- 
Wei Yang
Help You, Help Me

[-- Attachment #2: Type: text/html, Size: 3272 bytes --]

Re: One comment on the __release_region in kernel/resource.c

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

2011/10/3 Geert Uytterhoeven <geert@linux-m68k.org>

> On Mon, Oct 3, 2011 at 15:35, Wei Yang <weiyang.kernel@gmail.com> wrote:
> > 2011/10/3 Geert Uytterhoeven <geert@linux-m68k.org>
> >> On Mon, Oct 3, 2011 at 12:24, KAMEZAWA Hiroyuki
> >> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >> > 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
> >> >>    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
> >>                                              ^^^^^^^
> >> Do you really mean [50,59]?
> >
> > Yes.
> >>
> >> I don't think that's allowed, as the tree has [55,59], so you would
> >> release a
> >> larger region that allocated.
> >
> > So you mean the case I mentioned will not happen?
>
> Indeed, it should not happen.
> Actually I'm surprised it doesn't return an error code.
>
Do you think someone will take care of this?

>
> > Actually, I believe every developer should pass the resource region which
> > has been allocated.
> > While if some one made a mistake and pass a region which is not allocated
> > before and overlap
> > some "BUSY" region?
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker.
> But
> when I'm talking to journalists I just say "programmer" or something like
> that.
>                                 -- Linus Torvalds
>



-- 
Wei Yang
Help You, Help Me

[-- Attachment #2: Type: text/html, Size: 3246 bytes --]

Re: One comment on the __release_region in kernel/resource.c

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

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
> >    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

[-- Attachment #2: Type: text/html, Size: 2619 bytes --]