Re: [patch] Minor PAGG attach/detach semantic change for 2.6.11

On Tue, Oct 04, 2005 at 04:35:58PM +1000, Kingsley Cheung wrote:
> On Fri, Sep 30, 2005 at 03:58:58PM -0500, Erik Jacobson wrote:
> > Ok, I think I understand now.  You're saying since the process never
> > really was running, it doesn't make sense to run __pnotify_exit.
> > 
> > In __pnotify_fork, I think I agree with you.  If there was a failure 
> > forking, the kernel module wouldn't expect to be notified for exit.
> > 
> > Also, if the fork callout indicated that the process should fail as a
> > fork failure, I agree, the kernel module wouldn't expect the the
> > exit callout.
> > 
> > However, in fork.c's copy_process function, I'm not sure I'm comfortable
> > changing the pnotify_exit call there to pnotify_unsubscribe.  I don't want
> > to cause any sort of memory leak where structures a kernel module allocated
> > are not freed.  
> 
> No, that's definitely not what we want.
> 
> > 
> > However, we know if our own pnotify_fork call failed, that we don't have
> > anything to cleanup.  So I made that jump past the pnotify_exit in the
> > cleanup section.  But if anything after pnotify_fork fails, pnotify_exit
> > will be called.  This seems pretty normal for copy_process.
> > 
> > Does that sound ok?
> 
> I'm not sure about that.  What I was thinking of was calling
> unsubscribe in pnotify_fork for the process responsible for failing
> the fork.  Then the code in copy_process would jump to pnotify_exit
> and cleanup for any other remaining modules that have subscribed.
> This covers all modules and ensures every module that did not veto the
> fork has its exit function called.
> 
> > 
> > I'll post an updated patch shortly with this and some other feedback.
> 
> I'll wait for your updated patch and check it out to see.
> 

Erik,

I saw the latest patch you posted.  I think there is a bug in
copy_process() with respect to the veto clean up.  At the moment the
code is:

       retval = pnotify_fork(p, current);
       if (retval)
               goto bad_fork_cleanup_namespace;

Now if a veto occurs, this would jump over the pnotify_exit() and
never call it.  Client modules that may have subscribed successfully
would then not be cleaned up by pnotify_exit().  AFAICS successful
subscriptions are not cleaned up in pnotify_fork() either.

I suppose we could put the cleanup back into pnotify_exit() or we coul
change the above to become:

       retval = pnotify_fork(p, current);
       if (retval)
               goto bad_fork_cleanup_pnotify;

What do you think?
-- 
		Kingsley