On Wed, Sep 30, 2009 at 12:22:34PM -0400, Jarod Wilson wrote:
> On 09/18/2009 03:43 AM, Mel Gorman wrote:
>> On Thu, Sep 17, 2009 at 04:59:15PM -0400, Jarod Wilson wrote:
>>>>> The attached python script has been used successfully on Red Hat
>>>>> Enterprise Linux 5, Fedora 11 and Fedora 12, and is likely to work for
>>>>> other distros (though possibly with some minor tweaking required).
> ...
>>>> My preference if possible would be to integrate as much as possible into
>>>> hugeadm and have this script converted to using hugeadm where
>>>> appropriate.
>>>
>>> So based on earlier feedback from David Gibson, I rewrote it slightly to
>>> make more use of hugeadm and pagesize, but I've talked it over w/my
>>> manager, and have the approval to go ahead and work on integrating as
>>> much as possible of what this script does into hugeadm itself.
>>>
>>
>> Great stuff.
>
> Attaching a full diff that implements the bulk of the things that aren't
> terribly hard to add to hugeadm itself. Semi-sanely broken out patches
> available here:
>
> http://people.redhat.com/jwilson/misc/hugeadm-enhancements/
>
I am working from the broken-out patches, they're easier to read.
>
>>>> I think creating groups might be beyond the scope of hugeadm. This is
>>>> possibly the most distro-specific part of the entire script so I'd be a
>>>> little more wary of integrating it.
>>>
>>> Agreed. Creating users and groups definitely doesn't belong in hugeadm.
>>> My thought is that anything not belonging in there can still reside in
>>> an updated version of this script which does everything specific to huge
>>> pages using hugeadm. It'd be much more of a wrapper to hugeadm and
>>> {user,group}{add,mod} -- and possibly sysctl.
>
> Haven't yet rewritten the script, but it should only have to wrap
> hugeadm and the user/group add/mod bits now, I think...
>
Looks like it.
>> Perhaps there is some scope for libhugetlbfs installing silently the first
>> time and have a forced reinstallion present some configuration options such
>> as creating a group and adding users as this script does?
>
> I'm inclined to say no. At least in the RHEL world, people will
> primarily be installing via packages, and anything interactive at
> install/uninstall/reinstall/etc is pretty much no-go.
Figured as much. It's somewhat frowned upon and rare in the Debian world
as well.
> Now, we *could*
> theoretically have the package create something like a hugepage group at
> install time, and even set hugetlb_shm_group, but not in a persistent
> way (at least not w/o munging /etc/sysctl.conf directly from the package
> install scriptlet, which would also probably be frowned upon). I'm
> inclined to leave all of this to the user to configure after
> installation -- though with the possible aid of said script, once
> rewritten...
>
Sounds fair. No need to rock the boat until it is complained about.
>>>> hmm.... can't decide on this one. Not sure whether hugeadm should know to
>>>> to
>>>> make settings persist or if it should be recommended that hugeadm
>>>> invocations
>>>> be put into an rc script.
>>>
>>> Yeah, having hugeadm write to sysctl.conf doesn't sound like the best
>>> idea to me either. What about having hugeadm simply inform the user what
>>> sysctl settings they would need to add to have the settings persist?
>>>
>>
>> That makes sense. It could be suggested by --explain which I just
>> noticed has no manual page entry. I should fix that.
>
> I've added a bit to --set-recommended-shmmax and --set-shm-group to spit
> out a warning "add foo to /etc/sysctl.conf to make these settings
> persist" for now. Didn't add anything to --explain though.
>
It can be done as a follow-on.
>>> So in in the limits.conf case, its a stand-alone file in
>>> /etc/security/limits.d/, so maybe its okay to scribble on this file?
>>> Certainly less contentious than munging sysctl.conf anyway.
>>>
>>
>> I'd view them as being very similar. I think we should be able to
>> persist all settings or none at all. Maybe that's just me though.
>
> Ideally, yeah, all or none... But its a bit murkier, if in one case
> we're editing a system-wide file, vs. editing a file that could be part
> of the libhugetlbfs distribution itself. For example,
> /etc/security/limits.d/hugetlbfs.conf could be a file created by the
> libhugetlbfs rpm on RHEL, in which case, we're definitely free and clear
> to do with it as we please. But /etc/sysctl.conf isn't "ours". Bleah. We
> need an /etc/sysctl.d/hugetlbfs.conf. :)
>
> 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.
0001-hugeadm-show-amount-of-system-memory-in-explain-out.patch
Acked-by: Mel Gorman <m...@csn.ul.ie>
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.
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?
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.
0003-hugeadm-add-check_user-function-to-warn-if-user-is.patch
Return value of getgrgid is not being checked.
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.
0004-hugeadm-fix-typo.patch
Should be separate from the set but
Acked-by: Mel Gorman <m...@csn.ul.ie>
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?
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
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>
0008-hugeadm-add-support-for-setting-hugetlb_shm_group.patch
Acked-by: Mel Gorman <m...@csn.ul.ie>
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?
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.
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.
Thanks.
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
------------------------------------------------------------------------------
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
