On 23.10.2018 7:15, Joe Perches wrote:> On Mon, 2018-10-22 at 22:53 +0530, Arun KS wrote:
>> Remove managed_page_count_lock spinlock and instead use atomic
>> variables.
>
> Perhaps better to define and use macros for the accesses
> instead of specific uses of atomic_long_<inc/dec/read>
>
> Something like:
>
> #define totalram_pages() (unsigned long)atomic_long_read(&_totalram_pages)
or proper static inline
this code isn't so low level for breaking include dependencies with macro
> #define totalram_pages_inc() (unsigned long)atomic_long_inc(&_totalram_pages)
> #define totalram_pages_dec() (unsigned long)atomic_long_dec(&_totalram_pages)
these are void
conversion zone->managed_pages should be split into separate patch
[dropped bloated cc - my server rejects this mess]
On Mon, Oct 22, 2018 at 10:11 PM, Konstantin Khlebnikov
<[email protected]> wrote:
> On 23.10.2018 7:15, Joe Perches wrote:> On Mon, 2018-10-22 at 22:53 +0530,
> Arun KS wrote:
>>> Remove managed_page_count_lock spinlock and instead use atomic
>>> variables.
>>
>> Perhaps better to define and use macros for the accesses
>> instead of specific uses of atomic_long_<inc/dec/read>
>>
>> Something like:
>>
>> #define totalram_pages() (unsigned
>> long)atomic_long_read(&_totalram_pages)
>
> or proper static inline
> this code isn't so low level for breaking include dependencies with macro
BTW, I noticed a few places in the patch that did multiple evaluations
of totalram_pages. It might be worth fixing those prior to doing the
conversion, too. e.g.:
if (totalram_pages > something)
foobar(totalram_pages); <- value may have changed here
should, instead, be:
var = totalram_pages; <- get stable view of the value
if (var > something)
foobar(var);
-Kees
> [dropped bloated cc - my server rejects this mess]
Thank you -- I was struggling to figure out the best way to reply to this. :)
-Kees
--
Kees Cook
On 2018-10-24 01:34, Kees Cook wrote:
> On Mon, Oct 22, 2018 at 10:11 PM, Konstantin Khlebnikov
> <[email protected]> wrote:
>> On 23.10.2018 7:15, Joe Perches wrote:> On Mon, 2018-10-22 at 22:53
>> +0530,
>> Arun KS wrote:
>>>> Remove managed_page_count_lock spinlock and instead use atomic
>>>> variables.
>>>
>>> Perhaps better to define and use macros for the accesses
>>> instead of specific uses of atomic_long_<inc/dec/read>
>>>
>>> Something like:
>>>
>>> #define totalram_pages() (unsigned
>>> long)atomic_long_read(&_totalram_pages)
>>
>> or proper static inline
>> this code isn't so low level for breaking include dependencies with
>> macro
>
> BTW, I noticed a few places in the patch that did multiple evaluations
> of totalram_pages. It might be worth fixing those prior to doing the
> conversion, too. e.g.:
>
> if (totalram_pages > something)
> foobar(totalram_pages); <- value may have changed here
>
> should, instead, be:
>
> var = totalram_pages; <- get stable view of the value
> if (var > something)
> foobar(var);
Thanks for reviewing. Point taken.
>
> -Kees
>
>> [dropped bloated cc - my server rejects this mess]
>
> Thank you -- I was struggling to figure out the best way to reply to
> this. :)
I'm sorry for the trouble caused. Sent the email using,
git send-email --to-cmd="scripts/get_maintainer.pl -i"
0001-convert-totalram_pages-totalhigh_pages-and-managed_p.patch
Is this not a recommended approach?
Regards,
Arun
>
> -Kees
On Wed 24-10-18 10:47:52, Arun KS wrote:
> On 2018-10-24 01:34, Kees Cook wrote:
[...]
> > Thank you -- I was struggling to figure out the best way to reply to
> > this. :)
> I'm sorry for the trouble caused. Sent the email using,
> git send-email --to-cmd="scripts/get_maintainer.pl -i"
> 0001-convert-totalram_pages-totalhigh_pages-and-managed_p.patch
>
> Is this not a recommended approach?
Not really for tree wide mechanical changes. It is much more preferrable
IMHO to only CC people who should review the intention of the change
rather than each and every maintainer whose code is going to be changed.
This is a case by case thing of course but as soon as you see a giant CC
list from get_maintainer.pl then you should try to think twice to use
it. If not sure, just ask on the mailing list.
--
Michal Hocko
SUSE Labs
On Wed, 2018-10-24 at 08:15 +0200, Michal Hocko wrote:
> On Wed 24-10-18 10:47:52, Arun KS wrote:
> > On 2018-10-24 01:34, Kees Cook wrote:
> [...]
> > > Thank you -- I was struggling to figure out the best way to reply to
> > > this. :)
> > I'm sorry for the trouble caused. Sent the email using,
> > git send-email --to-cmd="scripts/get_maintainer.pl -i"
> > 0001-convert-totalram_pages-totalhigh_pages-and-managed_p.patch
> >
> > Is this not a recommended approach?
>
> Not really for tree wide mechanical changes. It is much more preferrable
> IMHO to only CC people who should review the intention of the change
> rather than each and every maintainer whose code is going to be changed.
> This is a case by case thing of course but as soon as you see a giant CC
> list from get_maintainer.pl then you should try to think twice to use
> it. If not sure, just ask on the mailing list.
Generally, it's better to use scripts to control
the --to-cmd and --cc-cmd options.
Something like what I detailed here:
https://lkml.org/lkml/2016/9/14/482
On Tue 23-10-18 23:26:16, Joe Perches wrote:
> On Wed, 2018-10-24 at 08:15 +0200, Michal Hocko wrote:
> > On Wed 24-10-18 10:47:52, Arun KS wrote:
> > > On 2018-10-24 01:34, Kees Cook wrote:
> > [...]
> > > > Thank you -- I was struggling to figure out the best way to reply to
> > > > this. :)
> > > I'm sorry for the trouble caused. Sent the email using,
> > > git send-email --to-cmd="scripts/get_maintainer.pl -i"
> > > 0001-convert-totalram_pages-totalhigh_pages-and-managed_p.patch
> > >
> > > Is this not a recommended approach?
> >
> > Not really for tree wide mechanical changes. It is much more preferrable
> > IMHO to only CC people who should review the intention of the change
> > rather than each and every maintainer whose code is going to be changed.
> > This is a case by case thing of course but as soon as you see a giant CC
> > list from get_maintainer.pl then you should try to think twice to use
> > it. If not sure, just ask on the mailing list.
>
> Generally, it's better to use scripts to control
> the --to-cmd and --cc-cmd options.
I would argue that it is better to use a common sense much more than
scripts.
--
Michal Hocko
SUSE Labs
On Wed, 2018-10-24 at 10:23 +0200, Michal Hocko wrote:
> On Tue 23-10-18 23:26:16, Joe Perches wrote:
> > On Wed, 2018-10-24 at 08:15 +0200, Michal Hocko wrote:
> > > On Wed 24-10-18 10:47:52, Arun KS wrote:
> > > > On 2018-10-24 01:34, Kees Cook wrote:
> > > [...]
> > > > > Thank you -- I was struggling to figure out the best way to reply to
> > > > > this. :)
> > > > I'm sorry for the trouble caused. Sent the email using,
> > > > git send-email --to-cmd="scripts/get_maintainer.pl -i"
> > > > 0001-convert-totalram_pages-totalhigh_pages-and-managed_p.patch
> > > >
> > > > Is this not a recommended approach?
> > >
> > > Not really for tree wide mechanical changes. It is much more preferrable
> > > IMHO to only CC people who should review the intention of the change
> > > rather than each and every maintainer whose code is going to be changed.
> > > This is a case by case thing of course but as soon as you see a giant CC
> > > list from get_maintainer.pl then you should try to think twice to use
> > > it. If not sure, just ask on the mailing list.
> >
> > Generally, it's better to use scripts to control
> > the --to-cmd and --cc-cmd options.
>
> I would argue that it is better to use a common sense much more than
> scripts.
Common sense isn't common.
Perhaps you could describe some guidelines you
use to determine what you think is sensible to
send patches to maintainers and reviewers and
appropriate mailing lists.
Then compare those to the rules in the scripts
I suggested.
My suggestions:
o send to all top-level listed maintainers and
reviewers in MAINTAINERS for the specific files
in a patch
o cc all maintainers and reviewers that are upstream
paths for the files in a patch
o cc all the upstream mailing lists for the files
in the patch.
o do not generally use git-history to exclude authors
of patches like drive-by/whitespace cleanups
Other advanced possibilities for patches that
modify specific and perhaps complex logic blocks:
o cc the people that are not maintainers that
have modified the specific blocks or functions
On Wed 24-10-18 01:39:18, Joe Perches wrote:
> On Wed, 2018-10-24 at 10:23 +0200, Michal Hocko wrote:
> > On Tue 23-10-18 23:26:16, Joe Perches wrote:
> > > On Wed, 2018-10-24 at 08:15 +0200, Michal Hocko wrote:
> > > > On Wed 24-10-18 10:47:52, Arun KS wrote:
> > > > > On 2018-10-24 01:34, Kees Cook wrote:
> > > > [...]
> > > > > > Thank you -- I was struggling to figure out the best way to reply to
> > > > > > this. :)
> > > > > I'm sorry for the trouble caused. Sent the email using,
> > > > > git send-email --to-cmd="scripts/get_maintainer.pl -i"
> > > > > 0001-convert-totalram_pages-totalhigh_pages-and-managed_p.patch
> > > > >
> > > > > Is this not a recommended approach?
> > > >
> > > > Not really for tree wide mechanical changes. It is much more preferrable
> > > > IMHO to only CC people who should review the intention of the change
> > > > rather than each and every maintainer whose code is going to be changed.
> > > > This is a case by case thing of course but as soon as you see a giant CC
> > > > list from get_maintainer.pl then you should try to think twice to use
> > > > it. If not sure, just ask on the mailing list.
> > >
> > > Generally, it's better to use scripts to control
> > > the --to-cmd and --cc-cmd options.
> >
> > I would argue that it is better to use a common sense much more than
> > scripts.
>
> Common sense isn't common.
But you cannot replace brain by a script.
Again, this is mostly a mechanical change quite internal to the MM
proper. Involving all the maintainers which happen to use totalram_pages
etc. are unlikely to need to know about this change. Sure there is some
chance of merge conflicts and that is where linux-next comes really
handy.
This is the best advice I can give here.
--
Michal Hocko
SUSE Labs