* question about link_mem_sections() @ 2011-12-12 12:35 Kay Sievers 2011-12-12 12:45 ` Robin Holt 0 siblings, 1 reply; 6+ messages in thread From: Kay Sievers @ 2011-12-12 12:35 UTC (permalink / raw) To: Robin Holt; +Cc: Greg Kroah-Hartmann, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: question about link_mem_sections() 2011-12-12 12:35 question about link_mem_sections() Kay Sievers @ 2011-12-12 12:45 ` Robin Holt 2011-12-12 13:11 ` Kay Sievers 0 siblings, 1 reply; 6+ messages in thread From: Robin Holt @ 2011-12-12 12:45 UTC (permalink / raw) To: Kay Sievers; +Cc: Robin Holt, Greg Kroah-Hartmann, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: question about link_mem_sections() 2011-12-12 12:45 ` Robin Holt @ 2011-12-12 13:11 ` Kay Sievers 2011-12-12 13:49 ` Robin Holt 0 siblings, 1 reply; 6+ messages in thread From: Kay Sievers @ 2011-12-12 13:11 UTC (permalink / raw) To: Robin Holt; +Cc: Greg Kroah-Hartmann, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: question about link_mem_sections() 2011-12-12 13:11 ` Kay Sievers @ 2011-12-12 13:49 ` Robin Holt 2011-12-22 1:18 ` Kay Sievers 0 siblings, 1 reply; 6+ messages in thread From: Robin Holt @ 2011-12-12 13:49 UTC (permalink / raw) To: Kay Sievers; +Cc: Robin Holt, Greg Kroah-Hartmann, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: question about link_mem_sections() 2011-12-12 13:49 ` Robin Holt @ 2011-12-22 1:18 ` Kay Sievers 2011-12-22 9:30 ` Robin Holt 0 siblings, 1 reply; 6+ messages in thread From: Kay Sievers @ 2011-12-22 1:18 UTC (permalink / raw) To: Robin Holt; +Cc: Greg Kroah-Hartmann, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: question about link_mem_sections() 2011-12-22 1:18 ` Kay Sievers @ 2011-12-22 9:30 ` Robin Holt 0 siblings, 0 replies; 6+ messages in thread From: Robin Holt @ 2011-12-22 9:30 UTC (permalink / raw) To: Kay Sievers, Jack Steiner, Russ Anderson Cc: Robin Holt, Greg Kroah-Hartmann, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-12-22 9:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-12 12:35 question about link_mem_sections() Kay Sievers 2011-12-12 12:45 ` Robin Holt 2011-12-12 13:11 ` Kay Sievers 2011-12-12 13:49 ` Robin Holt 2011-12-22 1:18 ` Kay Sievers 2011-12-22 9:30 ` Robin Holt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).