2018-10-23 05:35:00

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH] mm: convert totalram_pages, totalhigh_pages and managed_pages to atomic.

[Trimmed CC list + Julia - there is indeed no need to CC everybody maintain a
file you are updating for the change like this]

On Tue 23-10-18 10:16:51, Arun Sudhilal wrote:
> On Mon, Oct 22, 2018 at 11:41 PM Michal Hocko <[email protected]> wrote:
> >
> > On Mon 22-10-18 22:53:22, Arun KS wrote:
> > > Remove managed_page_count_lock spinlock and instead use atomic
> > > variables.
> >
>
> Hello Michal,
> > I assume this has been auto-generated. If yes, it would be better to
> > mention the script so that people can review it and regenerate for
> > comparision. Such a large change is hard to review manually.
>
> Changes were made partially with script. For totalram_pages and
> totalhigh_pages,
>
> find dir -type f -exec sed -i
> 's/totalram_pages/atomic_long_read(\&totalram_pages)/g' {} \;
> find dir -type f -exec sed -i
> 's/totalhigh_pages/atomic_long_read(\&totalhigh_pages)/g' {} \;
>
> For managed_pages it was mostly manual edits after using,
> find mm/ -type f -exec sed -i
> 's/zone->managed_pages/atomic_long_read(\&zone->managed_pages)/g' {}
> \;

I guess we should be able to use coccinelle for this kind of change and
reduce the amount of manual intervention to absolute minimum.
--
Michal Hocko
SUSE Labs


2018-10-23 06:13:52

by Julia Lawall

[permalink] [raw]
Subject: Re: [PATCH] mm: convert totalram_pages, totalhigh_pages and managed_pages to atomic.



On Tue, 23 Oct 2018, Michal Hocko wrote:

> [Trimmed CC list + Julia - there is indeed no need to CC everybody maintain a
> file you are updating for the change like this]
>
> On Tue 23-10-18 10:16:51, Arun Sudhilal wrote:
> > On Mon, Oct 22, 2018 at 11:41 PM Michal Hocko <[email protected]> wrote:
> > >
> > > On Mon 22-10-18 22:53:22, Arun KS wrote:
> > > > Remove managed_page_count_lock spinlock and instead use atomic
> > > > variables.
> > >
> >
> > Hello Michal,
> > > I assume this has been auto-generated. If yes, it would be better to
> > > mention the script so that people can review it and regenerate for
> > > comparision. Such a large change is hard to review manually.
> >
> > Changes were made partially with script. For totalram_pages and
> > totalhigh_pages,
> >
> > find dir -type f -exec sed -i
> > 's/totalram_pages/atomic_long_read(\&totalram_pages)/g' {} \;
> > find dir -type f -exec sed -i
> > 's/totalhigh_pages/atomic_long_read(\&totalhigh_pages)/g' {} \;
> >
> > For managed_pages it was mostly manual edits after using,
> > find mm/ -type f -exec sed -i
> > 's/zone->managed_pages/atomic_long_read(\&zone->managed_pages)/g' {}
> > \;
>
> I guess we should be able to use coccinelle for this kind of change and
> reduce the amount of manual intervention to absolute minimum.

Coccinelle looks like it would be desirable, especially in case the word
zone is not always used.

Arun, please feel free to contact me if you want to try it and need help.

julia

2018-10-23 07:24:28

by Arun Sudhilal

[permalink] [raw]
Subject: Re: [PATCH] mm: convert totalram_pages, totalhigh_pages and managed_pages to atomic.

On Tue, Oct 23, 2018 at 12:11 PM Julia Lawall <[email protected]> wrote:
>
>
>
> On Tue, 23 Oct 2018, Michal Hocko wrote:
>
> > [Trimmed CC list + Julia - there is indeed no need to CC everybody maintain a
> > file you are updating for the change like this]
> >
> > On Tue 23-10-18 10:16:51, Arun Sudhilal wrote:
> > > On Mon, Oct 22, 2018 at 11:41 PM Michal Hocko <[email protected]> wrote:
> > > >
> > > > On Mon 22-10-18 22:53:22, Arun KS wrote:
> > > > > Remove managed_page_count_lock spinlock and instead use atomic
> > > > > variables.
> > > >
> > >
> > > Hello Michal,
> > > > I assume this has been auto-generated. If yes, it would be better to
> > > > mention the script so that people can review it and regenerate for
> > > > comparision. Such a large change is hard to review manually.
> > >
> > > Changes were made partially with script. For totalram_pages and
> > > totalhigh_pages,
> > >
> > > find dir -type f -exec sed -i
> > > 's/totalram_pages/atomic_long_read(\&totalram_pages)/g' {} \;
> > > find dir -type f -exec sed -i
> > > 's/totalhigh_pages/atomic_long_read(\&totalhigh_pages)/g' {} \;
> > >
> > > For managed_pages it was mostly manual edits after using,
> > > find mm/ -type f -exec sed -i
> > > 's/zone->managed_pages/atomic_long_read(\&zone->managed_pages)/g' {}
> > > \;
> >
> > I guess we should be able to use coccinelle for this kind of change and
> > reduce the amount of manual intervention to absolute minimum.
>
> Coccinelle looks like it would be desirable, especially in case the word
> zone is not always used.
>
> Arun, please feel free to contact me if you want to try it and need help.

Thanks Julia. I m starting with,
http://coccinelle.lip6.fr/papers/backport_edcc15.pdf

Regards,
Arun
>
> julia

2018-10-25 05:33:51

by Arun KS

[permalink] [raw]
Subject: Re: [PATCH] mm: convert totalram_pages, totalhigh_pages and managed_pages to atomic.

On 2018-10-23 11:43, Julia Lawall wrote:
> On Tue, 23 Oct 2018, Michal Hocko wrote:
>
>> [Trimmed CC list + Julia - there is indeed no need to CC everybody
>> maintain a
>> file you are updating for the change like this]
>>
>> On Tue 23-10-18 10:16:51, Arun Sudhilal wrote:
>> > On Mon, Oct 22, 2018 at 11:41 PM Michal Hocko <[email protected]> wrote:
>> > >
>> > > On Mon 22-10-18 22:53:22, Arun KS wrote:
>> > > > Remove managed_page_count_lock spinlock and instead use atomic
>> > > > variables.
>> > >
>> >
>> > Hello Michal,
>> > > I assume this has been auto-generated. If yes, it would be better to
>> > > mention the script so that people can review it and regenerate for
>> > > comparision. Such a large change is hard to review manually.
>> >
>> > Changes were made partially with script. For totalram_pages and
>> > totalhigh_pages,
>> >
>> > find dir -type f -exec sed -i
>> > 's/totalram_pages/atomic_long_read(\&totalram_pages)/g' {} \;
>> > find dir -type f -exec sed -i
>> > 's/totalhigh_pages/atomic_long_read(\&totalhigh_pages)/g' {} \;
>> >
>> > For managed_pages it was mostly manual edits after using,
>> > find mm/ -type f -exec sed -i
>> > 's/zone->managed_pages/atomic_long_read(\&zone->managed_pages)/g' {}
>> > \;
>>
>> I guess we should be able to use coccinelle for this kind of change
>> and
>> reduce the amount of manual intervention to absolute minimum.
>
> Coccinelle looks like it would be desirable, especially in case the
> word
> zone is not always used.
>
> Arun, please feel free to contact me if you want to try it and need
> help.
Hello Julia,

I was able to come up .cocci for replacing managed_pages,

@@
struct zone *z;
@@
(
z->managed_pages = ...
|
- z->managed_pages
+ atomic_long_read(&z->managed_pages)
)

@@
struct zone *z;
expression e1;
@@
- z->managed_pages = e1
+ atomic_long_set(&z->managed_pages, e1)

@@
expression e,e1;
@@
- e->managed_pages += e1
+ atomic_long_add(e1, &e->managed_pages)

@@
expression z;
@@
- z.managed_pages
+ atomic_long_read(&z.managed_pages)

But I m not able to use same method for unsigned long
variables(totalram_pages)

@@
unsigned long totalram_pages;
@@
(
totalram_pages = ...
|
-totalram_pages
+atomic_long_read(&totalram_pages)
)

This throws error,

spatch test1.cocci mm/page_alloc.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: mm/page_alloc.c

previous modification:
MINUS
>>> atomic_long_read(&rule starting on line 1:totalram_pages)

According to environment 1:
rule starting on line 1.totalram_pages -> page_idx ^ (1 <<
order)

current modification:
MINUS
>>> atomic_long_read(&rule starting on line 1:totalram_pages)

According to environment 1:
rule starting on line 1.totalram_pages -> page_idx

Fatal error: exception Failure("rule starting on line 1: already tagged
token:
C code context
File "mm/internal.h", line 175, column 8, charpos = 5368
around = 'page_idx', whole content = return page_idx ^ (1 <<
order);")

Regards,
Arun


>
> julia