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

[Erik Jacobson <erikj@xxxxxxx>]

Kingsley - my first attempt skipped the list, sorry.

> While testing the propagation of pagg_attach errors to fork() I
> noticed that the detach callback is called again for the client

I'm sorry it's taking a while for me to get back to you.
I had kicked your patch around to a couple people internally and
I think we want to investigate the error path more before we
take it as part of the PAGG patch.

Does anybody else on the list have thoughts on this change?

Thanks for the submission. I'd like to do a bit more research.

> responsible for the error.  Perhaps you may have a different opinion
> on this, but IMHO this is unnecessary as the client passing the
> failure error up should have already cleaned up any data in its pagg
> structure during its attach callback.
> 
> I've atttached a patch that applies (with some fuzz) correctly to
> 2.6.11 that avoids this unncessary call to the client's detach
> callback.  Please consider applying.
> 
> Thanks, 
> -- 
>               Kingsley

> Index: kernel/fork.c
> ===================================================================
> RCS file: /export/cvsroot/SLES9_kernel/kernel/fork.c,v
> retrieving revision 1.1.1.3.12.1
> diff -u -r1.1.1.3.12.1 fork.c
> --- kernel/fork.c     27 May 2005 05:17:01 -0000      1.1.1.3.12.1
> +++ kernel/fork.c     17 Jun 2005 00:39:33 -0000
> @@ -1086,7 +1086,7 @@
>       if (sigismember(&current->pending.signal, SIGKILL)) {
>               write_unlock_irq(&tasklist_lock);
>               retval = -EINTR;
> -             goto bad_fork_cleanup_namespace;
> +             goto bad_fork_cleanup_pagg;
>       }
>  
>       /* CLONE_PARENT re-uses the old parent */
> @@ -1107,7 +1107,7 @@
>                       spin_unlock(&current->sighand->siglock);
>                       write_unlock_irq(&tasklist_lock);
>                       retval = -EAGAIN;
> -                     goto bad_fork_cleanup_namespace;
> +                     goto bad_fork_cleanup_pagg;
>               }
>               p->group_leader = current->group_leader;
>  
> @@ -1149,8 +1149,9 @@
>               return ERR_PTR(retval);
>       return p;
>  
> -bad_fork_cleanup_namespace:
> +bad_fork_cleanup_pagg:
>       pagg_detach(p);
> +bad_fork_cleanup_namespace:
>       exit_namespace(p);
>  bad_fork_cleanup_mm:
>       exit_mm(p);
> Index: kernel/pagg.c
> ===================================================================
> RCS file: /export/cvsroot/SLES9_kernel/kernel/Attic/pagg.c,v
> retrieving revision 1.1.1.1.12.1
> diff -u -r1.1.1.1.12.1 pagg.c
> --- kernel/pagg.c     27 May 2005 05:17:01 -0000      1.1.1.1.12.1
> +++ kernel/pagg.c     17 Jun 2005 00:39:33 -0000
> @@ -410,14 +410,15 @@
>                       goto error_return;
>               }
>               ret = to_pagg->hook->attach(to_task, to_pagg, from_pagg->data);
> -
> -             if (ret < 0) {
> -                     /* Propagates to copy_process as a fork failure */
> -                     goto error_return;
> -             }
> -             else if (ret > 0) {
> -                     /* Success, but attach function pointer doesn't want 
> grouping */
> +             if (ret) {
> +                     /* Negative error propagates to copy_process
> +                      * as a fork failure.  Otherwise we have
> +                      * success, but the attach function pointer
> +                      * doesn't want grouping. 
> +                      */
>                       pagg_free(to_pagg);
> +                     if (ret < 0)
> +                             goto error_return;
>               }
>       }
>  

--
Erik Jacobson - Linux System Software - Silicon Graphics - Eagan, Minnesota