lkml.org 
[lkml]   [2011]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V2 2/2] mm: Extend memory hotplug API to allow memory hotplug in virtual machines
On Tue, May 03, 2011 at 09:25:52AM -0700, Dave Hansen wrote:
> On Mon, 2011-05-02 at 23:49 +0200, Daniel Kiper wrote:
> > +int register_online_page_callback(online_page_callback_t callback)
> > +{
> > + int rc = -EPERM;
> > +
> > + lock_memory_hotplug();
> > +
> > + if (online_page_callback == generic_online_page) {
> > + online_page_callback = callback;
> > + rc = 0;
> > + }
> > +
> > + unlock_memory_hotplug();
> > +
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(register_online_page_callback);
>
> -EPERM is a bit uninformative here. How about -EEXIST, plus a printk?

EEXIST means File exists (POSIX.1). It could be misleading. That is why
I decided to use EPERM. I could not find any better choice. I think another
choice is EINVAL (not the best one in my opinion). Additionally, I am not
sure it should have printk. I think it is role of caller to notify (or not)
about possible errors.

> I also don't seen the real use behind having a "register" that can only
> take a single callback. At worst, it should be
> "set_online_page_callback()" so it's more apparent that there can only
> be one of these.

OK.

> > +int unregister_online_page_callback(online_page_callback_t callback)
> > +{
> > + int rc = -EPERM;
> > +
> > + lock_memory_hotplug();
> > +
> > + if (online_page_callback == callback) {
> > + online_page_callback = generic_online_page;
> > + rc = 0;
> > + }
> > +
> > + unlock_memory_hotplug();
> > +
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(unregister_online_page_callback);
>
> Again, -EPERM is a bad code here. -EEXIST, perhaps? It also deserves a
> WARN_ON() or a printk on failure here.

Please look above.

> Your changelog doesn't mention, but what ever happened to doing
> something dirt-simple like this? I have a short memory.

Andrew Morton complained about (ab)use of notifiers. He suggested
to use callback machanism (I could not find any better solution
in Linux Kernel). He convinced me.

Daniel


\
 
 \ /
  Last update: 2011-05-03 22:17    [W:0.060 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site