On 2019-01-09 14:10, Michal Hocko wrote:
On Wed 09-01-19 13:58:50, Arun KS wrote:
On 2019-01-09 13:07, Michal Hocko wrote:
> On Wed 09-01-19 11:28:52, Arun KS wrote:
> > On 2019-01-08 23:43, Michal Hocko wrote:
> > > On Tue 08-01-19 09:56:09, Alexander Duyck wrote:
> > > > On Fri, 2019-01-04 at 10:31 +0530, Arun KS wrote:
> > > [...]
> > > > > static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> > > > > void *arg)
> > > > > {
> > > > > - unsigned long i;
> > > > > unsigned long onlined_pages = *(unsigned long *)arg;
> > > > > - struct page *page;
> > > > >
> > > > > if (PageReserved(pfn_to_page(start_pfn)))
> > > > > - for (i = 0; i < nr_pages; i++) {
> > > > > - page = pfn_to_page(start_pfn + i);
> > > > > - (*online_page_callback)(page);
> > > > > - onlined_pages++;
> > > > > - }
> > > > > + onlined_pages = online_pages_blocks(start_pfn, nr_pages);
> > > >
> > > > Shouldn't this be a "+=" instead of an "="? It seems like you are
> > > > going
> > > > to lose your count otherwise.
> > >
> > > You are right of course. I should have noticed during the review.
> > > Thanks!
> >
> > I think we don't need to. The caller function is setting
> > onlined_pages = 0
> > before calling online_pages_range().
> > And there are no other reference to online_pages_range other than from
> > online_pages().
>
> Are you missing that we accumulate onlined_pages via
> *(unsigned long *)arg = onlined_pages;
> in online_pages_range?
In my testing I didn't find any problem. To match the code being
replaced
and to avoid any corner cases, it is better to use +=
Will update the patch.
Have you checked that the number of present pages both in the zone and
the node is correct because I fail to see how that would be possible.
Yes they are showing correct values.
Previous value of cat /proc/zoneinfo,
Node 0, zone Normal
pages free 65492
min 300
low 375
high 450
spanned 65536
present 65536
managed 65536
Value after hotadd,
Node 0, zone Normal
pages free 129970
min 518
low 649
high 780
spanned 983040
present 131072
managed 131072
I added prints in online_pages_range function.
It will be called once per online of a section and the arg value is
always set to 0 while entering online_pages_range.
/sys/devices/system/memory # echo online > memory16/state
[ 52.956558] online_pages_range start_pfn = 100000 nr_pages = 65536
arg = 0
[ 52.964104] Built 1 zonelists, mobility grouping on. Total pages:
187367
[ 52.964828] Policy zone: Normal
But still I'll change to += to match with the previous code.
Regards,
Arun
