> From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
Thanks, Ian, for taking the time for review! Comments below.
> > Since this is the first time I've ported this code to a kernel
> > later than 2.6.34(?), and since this code hasn't gotten formal
> > review even though it's been floating about in various forms
> > for 18 months, I thought I would post it publicly for review,
> > rather than just ask for a pull. (I'll put it in a git tree after
> > a round or two of feedback.)
>
> checkpatch.pl says:
>
> total: 8 errors, 14 warnings, 395 lines checked
>
> most of them look valid to me. (I commented on a few below before it
> became clear you hadn't run it yourself)
Oops, yes, sorry, I should have done that first :-}
> > +unsigned long frontswap_inertia_counter = 0;
>
> static?
Yes, will fix.
> > + //frontswap_inertia_counter = balloon_stats.frontswap_inertia;
>
> Please drop this.
Will fix.
> > + extern unsigned long vm_get_committed_as(void);
>
> In a header please.
In an ideal world, yes, see below.
> > + balloon_stats.frontswap_inertia = 3;
> > +#endif
> > + schedule_delayed_work(&selfballoon_worker,
> > + balloon_stats.selfballoon_interval * HZ);
> > +#endif
>
> balloon_init already has paragraphs initialising balloon_stats -- this
> should go up with them I think.
OK, will move.
> > +static ssize_t show_selfballooning(struct sys_device *dev, struct
> > sysdev_attribute *attr,
> > + char *buf)
> > +{
> > + return sprintf(buf, "%d\n", balloon_stats.selfballooning_enabled);
> > +}
>
> For the show_* you define here you could largely use BALLOON_SHOW to
> define the function, I think.
Thanks, will take a look at that. I think when I first wrote
this, that wasn't the case but may work now.
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -89,6 +89,13 @@ int sysctl_overcommit_ratio = 50; /* default is 50% */
> > int sysctl_max_map_count __read_mostly = DEFAULT_MAX_MAP_COUNT;
> > struct percpu_counter vm_committed_as;
> >
> > +unsigned long vm_get_committed_as(void)
> > +{
> > + return percpu_counter_read_positive(&vm_committed_as);
> > +
> > +}
> > +EXPORT_SYMBOL(vm_get_committed_as);
> > +
>
> This needs to be split out and go upstream via the mm maintainers (with
> an extern in a header, not a C file).
Although you're right, as I am fresh off a 2-1/2 year odyssey of
upstreaming cleancache, AND since this is almost certainly Xen
specific AND since there will likely be some changes over time
which could conceivably make this unnecessary, I would be content
with carrying this in a Xen-only tree for the foreseeable future.
> You add a lot of code to {xen-,}balloon.c which is entirely encased in
> #ifdef CONFIG_THIS_AND_THAT, without very much interaction with existing
> code in those files. That suggests to me that the code should live in
> its own file.
>
> Some careful consideration needs to go into the split between balloon.c
> (core kernel functionality, non-modular), xen-balloon.c (user
> interfaces, potentially modular, not actually at the moment) and
> $NEWFILE.c.
>
> In fact it seems as if all this functionality is a second user of the
> core functionality exposed by balloon.c which is independent of the
> existing xen-balloon.c user of that API. Therefore it should all live in
> a new module next to xen-balloon.c module rather than be intertwined
> with both xen-balloon.c and balloon.c.
Could be. I think when I first wrote this, it would have required
some more things to be externified, so I didn't bother. Will
take a look again.
I was wondering why xen-balloon.c and balloon.c are separate to
begin with? It forces a global variable balloon_stats which could
be static otherwise. Though it might be possible for a kernel to be
built with only one of the two to save space, is there any good reason?
Or is it just because the file was getting too big?
I note that both of them have a balloon_init and both pr_info
a (not quite identical) message.
> Is there any particular reason this (all) needs to be in the kernel at
> all? Can a userspace daemon, using (possibly new) statistics exported
> via /sys and /proc plus the existing balloon interfaces not implement
> much the same thing? One nice advantage of doing that is that a
> userspace daemon can more easily implement complex (or multiple)
> algorithms, tune them etc. If there is a good reason for this to be in
> kernel I think it should be expanded upon in the commit message.
Will answer separately since I see this part of the thread has
already been continued.
Thanks again!
Dan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home