On 10/01/2009 09:07 AM, Mel Gorman wrote: > On Wed, Sep 30, 2009 at 12:22:34PM -0400, Jarod Wilson wrote: ... >> So hopefully, I've not butchered anything *too* badly... >> > > The entire diff is a bit of a mouthful so here is a patch-by-patch > commentary. It might be easier to post as a threaded patch series the > next time.
Yeah, probably should have done that in the first place, but the series was only semi-sanely broken up, as you discovered. ;) Doing too many things at once. Round two will be more sanely organized, and sent to the list as a threaded patch series. > 0002-hugeadm-add-initial-support-for-showing-and-setting.patch > Minor, but I'd recommend renaming get_recommended_shmmax to > recommended_shmmax() so it roughly matches the same pair of functions > for setting min_free_kbytes. recommended_shmmax() should comment > why it has a recommended value. Will do. > Is it really a good idea fix shmmax as the total of maximum > memory. As this is about hugepages, would a better value for shmmax > be the maximum number of hugepages that can be allocated? i.e. > all statically allocate hugepages + the allower overcommit? > > Similarly, should check_shmmax() warn warn when shmmax is less > than the number of hugepages that can be allocated? This does sound much more sane. However, I'm presently clueless exactly how the max number of hugepages that can be allocated is determined... The later patch that saves 2G or half of memory was sort of a WAG at "leaving enough for the rest of the system", but if there's something actually built-in that will tell me the upper limit for how much we can allocate for hugepages, using that would definitely be better. > In the text of shmmax(), it makes mention of the "maximum heap size" > which is somewhat specific to JVMs. It would be better if it referred > to largest shared memory segment, possibly giving the heap size in > a JVM as an example. Yeah, the "maximum heap size" bit was in fact heisted from an internal doc specific to setting up jboss for use with huge pages, and that's where the current shmmax settings came from as well... > 0003-hugeadm-add-check_user-function-to-warn-if-user-is.patch > > Return value of getgrgid is not being checked. Whoops. Adding a if (!grp) warn that an invalid gid is set. > The group check is also incomplete. If the hugetlb_shm_group > is the user-only group, then the pw_gid in struct passwd will > contain the group id but it will not necessarily be in the > list returned by getgwuid. For example > > $ grep ^mel /etc/group > mel:x:1000: > > mel is not in the group but > > $ grep ^mel /etc/passwd > mel:x:1000:1000:mel,,,:/home/mel:/bin/bash > > See, my gid is 1000. > > So this needs a bit more work. Gack. Good catch. Working on this one now... > 0004-hugeadm-fix-typo.patch > Should be separate from the set but > > Acked-by: Mel Gorman<m...@csn.ul.ie> Will break it out and send it by itself in a bit. > 0005-hugeadm-trim-recommended-shmmax-size.patch > > Minimally should be merged with patch 2. The "less 2G" feels a bit > arbitrary. How do you feel about the suggestion above on basing it > on the maximum number of hugepages that can possibly be in use by > the system? Works for me. Will hunt around to see where I can extract that maximum number from, but hints would be much appreciated. > 0006-hugeadm-suggest-sysctl.conf-addition-for-persistent.patch > > Maybe make this INFO level and suppress by default? The "done" > thing with utilities AFAIK is to be silent when successful Yeah, that sounds good. > 0007-hugeadm-support-pool-size-min-DEFAULT-2G-syntax.patch > The "Returning page count of" message should be DEBUG. Similar > for the page_size message. With those changed, it's a very nice > improvement to the utility. > > Acked-by: Mel Gorman<m...@csn.ul.ie> Was pretty happy with how this one turned out myself. I'll flip those messages to DEBUG for round 2. > 0009-hugeadm-print-note-on-how-to-make-hugetlb_shm_group.patch > > I have similar concertns with this as Patch 6. Maybe it > could be made part of --explain to say how settings > can be persisted? How 'bout we make it an INFO-level print when its actually set, and add spew to --explain for both this and shmmax settings? But if we do, should we go so far as to try to look and see if they have already been added to sysctl.conf, and suppress the message if they have? Or can we call just printing out that they need to be in there for persistence "Good Enough"? > 0010-hugeadm-update-help-output-with-new-switches-and-sy.patch > > These should be part of each of the patches that introduce > the switches. > > 0011-hugeadm-update-man-page.patch > Similar, these should be part of the patches that introduce > the switches. Yeah, will put these in their proper places. Side-effect of forgetting all about the docs until the very end. > Overall, this is looking in good shape as a first cut. while I think another > revision would do no harm, I do not believe any of the suggestions will pose > a serious difficulty. Yeah, it actually turned out to be a fair amount less work than I'd initially anticipated, and nothing that was terribly complex. I'll get an updated and more sanely organized patch series out as soon as I can. > Thanks. No problem, thanks much for the review! -- Jarod Wilson ja...@redhat.com ------------------------------------------------------------------------------ Come build with us! The BlackBerry® Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9-12, 2009. Register now! http://p.sf.net/sfu/devconf _______________________________________________ Libhugetlbfs-devel mailing list Libhugetlbfs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
