RE: [PATCH] [linux-2.6.39.x for xen] tmem: self-ballooning and frontswap-selfshrinking

[Dan Magenheimer <dan.magenheimer@oracle.com>]

> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]

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