question about link_mem_sections()

question about link_mem_sections()

[Kay Sievers]

Robin,

you added find_memory_block_hinted() with:
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=63d027a63888e993545d10fdfe4107d543f01bca

I try to understand what's going on here, because we need to switch
away from 'struct sysdev'.

In the loop over the node data you call find_memory_block_hinted() in
a row, which might all take a reference. At the end of the section you
drop only the last reference of the iteration. The code before your
change dropped all references inside the loop.

Could you please explain the intended behaviour?

If all is right, we should at least move the now wrong comment where is belongs.

Thanks,
Kay

Re: question about link_mem_sections()

[Robin Holt]

On Mon, Dec 12, 2011 at 01:35:50PM +0100, Kay Sievers wrote:
> Robin,
> 
> you added find_memory_block_hinted() with:
>   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=63d027a63888e993545d10fdfe4107d543f01bca
> 
> I try to understand what's going on here, because we need to switch
> away from 'struct sysdev'.
> 
> In the loop over the node data you call find_memory_block_hinted() in
> a row, which might all take a reference. At the end of the section you
> drop only the last reference of the iteration. The code before your
> change dropped all references inside the loop.
> 
> Could you please explain the intended behaviour?
> 
> If all is right, we should at least move the now wrong comment where is belongs.

Take a look at that commit's parent's parent:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=c25d1dfbd403209025df41a737f82ce8f43d93f5

This adds the kset_find_obj_hinted() function.  That function expects
the kobject_get to have already been done on the object passed in and
will release that reference when it advances to the next object.

Within the loop of the link_mem_sections(), we need to retain the
kobject_get reference between calls to find_memory_block_hinted() and
rely upon that functions call to kset_find_obj_hinted() to release all
but the last reference.

I hope that explains things.  If not, please feel free to ping me again.

Thanks,
Robin

Re: question about link_mem_sections()

[Kay Sievers]

On Mon, Dec 12, 2011 at 13:45, Robin Holt <holt@sgi.com> wrote:
> On Mon, Dec 12, 2011 at 01:35:50PM +0100, Kay Sievers wrote:
>> you added find_memory_block_hinted() with:
>>   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=63d027a63888e993545d10fdfe4107d543f01bca
>>
>> I try to understand what's going on here, because we need to switch
>> away from 'struct sysdev'.
>>
>> In the loop over the node data you call find_memory_block_hinted() in
>> a row, which might all take a reference. At the end of the section you
>> drop only the last reference of the iteration. The code before your
>> change dropped all references inside the loop.
>>
>> Could you please explain the intended behaviour?
>>
>> If all is right, we should at least move the now wrong comment where is belongs.
>
> Take a look at that commit's parent's parent:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=c25d1dfbd403209025df41a737f82ce8f43d93f5
>
> This adds the kset_find_obj_hinted() function.  That function expects
> the kobject_get to have already been done on the object passed in and
> will release that reference when it advances to the next object.
>
> Within the loop of the link_mem_sections(), we need to retain the
> kobject_get reference between calls to find_memory_block_hinted() and
> rely upon that functions call to kset_find_obj_hinted() to release all
> but the last reference.
>
> I hope that explains things.  If not, please feel free to ping me again.

Gah, what a twisted logic. That looks more like a pretty specialized
implementation of an iterator and not a generic kobject 'find'
function, when it implicitly mangles the hint. :)

Anyway, sounds fine, only the 'dead' and misleading comment in
link_mem_sections should move out of the loop, right?

Thanks,
Kay

Re: question about link_mem_sections()

[Robin Holt]

On Mon, Dec 12, 2011 at 02:11:28PM +0100, Kay Sievers wrote:
> On Mon, Dec 12, 2011 at 13:45, Robin Holt <holt@sgi.com> wrote:
> > On Mon, Dec 12, 2011 at 01:35:50PM +0100, Kay Sievers wrote:
> >> you added find_memory_block_hinted() with:
> >>   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=63d027a63888e993545d10fdfe4107d543f01bca
> >>
> >> I try to understand what's going on here, because we need to switch
> >> away from 'struct sysdev'.
> >>
> >> In the loop over the node data you call find_memory_block_hinted() in
> >> a row, which might all take a reference. At the end of the section you
> >> drop only the last reference of the iteration. The code before your
> >> change dropped all references inside the loop.
> >>
> >> Could you please explain the intended behaviour?
> >>
> >> If all is right, we should at least move the now wrong comment where is belongs.
> >
> > Take a look at that commit's parent's parent:
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=c25d1dfbd403209025df41a737f82ce8f43d93f5
> >
> > This adds the kset_find_obj_hinted() function.  That function expects
> > the kobject_get to have already been done on the object passed in and
> > will release that reference when it advances to the next object.
> >
> > Within the loop of the link_mem_sections(), we need to retain the
> > kobject_get reference between calls to find_memory_block_hinted() and
> > rely upon that functions call to kset_find_obj_hinted() to release all
> > but the last reference.
> >
> > I hope that explains things.  If not, please feel free to ping me again.
> 
> Gah, what a twisted logic. That looks more like a pretty specialized
> implementation of an iterator and not a generic kobject 'find'
> function, when it implicitly mangles the hint. :)
> 
> Anyway, sounds fine, only the 'dead' and misleading comment in
> link_mem_sections should move out of the loop, right?

Dead comment should go.  If you want to rework the logic, that is fine
with me as well.  The change did make a huge difference in boot time on
a large memory machine.  IIRC, it was something like 30 or 45 minutes
down to .27 seconds or something crazy like that.  There were two spots
that needed the speedup.

If you do decide to rework that logic, we can retest for the next couple
months.  We do not normally have a 16TB test machine lying around (IIRC,
the memory in something like that costs a couple million dollars), but we
do fortunately have one right now until the latter part of next quarter.

Robin

Re: question about link_mem_sections()

[Kay Sievers]

On Mon, Dec 12, 2011 at 14:49, Robin Holt <holt@sgi.com> wrote:
> On Mon, Dec 12, 2011 at 02:11:28PM +0100, Kay Sievers wrote:

> If you do decide to rework that logic, we can retest for the next couple
> months.  We do not normally have a 16TB test machine lying around (IIRC,
> the memory in something like that costs a couple million dollars), but we
> do fortunately have one right now until the latter part of next quarter.

The rework of the sys_devices with the goal to finally removal of all
sysdev stuff is in Greg's tree now:
  http://git.kernel.org/?p=linux/kernel/git/gregkh/driver-core.git;a=shortlog;h=refs/heads/driver-core-next
It will be available in linux-next.

The hinted search moved from the 'kobject' to the 'bus' as:
  dev = subsys_find_device_by_id(&memory_subsys, block_id, hintdev);

Would be nice if you can eventually give it a try, on the big box, to
make sure we didn't break anything.

Thanks a lot,
Kay

Re: question about link_mem_sections()

[Robin Holt]

On Thu, Dec 22, 2011 at 02:18:47AM +0100, Kay Sievers wrote:
> On Mon, Dec 12, 2011 at 14:49, Robin Holt <holt@sgi.com> wrote:
> > On Mon, Dec 12, 2011 at 02:11:28PM +0100, Kay Sievers wrote:
> 
> > If you do decide to rework that logic, we can retest for the next couple
> > months.  We do not normally have a 16TB test machine lying around (IIRC,
> > the memory in something like that costs a couple million dollars), but we
> > do fortunately have one right now until the latter part of next quarter.
> 
> The rework of the sys_devices with the goal to finally removal of all
> sysdev stuff is in Greg's tree now:
>   http://git.kernel.org/?p=linux/kernel/git/gregkh/driver-core.git;a=shortlog;h=refs/heads/driver-core-next
> It will be available in linux-next.
> 
> The hinted search moved from the 'kobject' to the 'bus' as:
>   dev = subsys_find_device_by_id(&memory_subsys, block_id, hintdev);
> 
> Would be nice if you can eventually give it a try, on the big box, to
> make sure we didn't break anything.

If it is in linux-next, we may already have it.

Russ or Jack, IIRC, you had been doing some testing with linux-next
on the 16TB machine.  Is that correct?  If so, do you happen to have a
timed boot log that we could look at?

Thanks,
Robin