[PATCH -mm] coredump: format_corename: don't append .%pid if multi-threaded

[PATCH -mm] coredump: format_corename: don't append .%pid if multi-threaded

[Oleg Nesterov]

If the coredumping is multi-threaded, format_corename() appends .%pid
to the corename. This was needed before the proper multi-thread core
dump support, now all the threads in the mm go into a single unified
core file.

Remove this special case, it is not even documented and we have "%p"
and core_uses_pid.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 26-rc2/fs/exec.c~FORMAT_CORENAME_NO_MT_PID	2008-07-22 15:42:15.000000000 +0400
+++ 26-rc2/fs/exec.c	2008-07-22 15:46:04.000000000 +0400
@@ -1373,7 +1373,7 @@ EXPORT_SYMBOL(set_binfmt);
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
  */
-static int format_corename(char *corename, int nr_threads, long signr)
+static int format_corename(char *corename, long signr)
 {
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
@@ -1480,8 +1480,7 @@ static int format_corename(char *corenam
 	 * If core_pattern does not include a %p (as is the default)
 	 * and core_uses_pid is set, then .%pid will be appended to
 	 * the filename. Do not do this for piped commands. */
-	if (!ispipe && !pid_in_pattern
-	    && (core_uses_pid || nr_threads)) {
+	if (!ispipe && !pid_in_pattern && core_uses_pid) {
 		rc = snprintf(out_ptr, out_end - out_ptr,
 			      ".%d", task_tgid_vnr(current));
 		if (rc > out_end - out_ptr)
@@ -1745,7 +1744,7 @@ int do_coredump(long signr, int exit_cod
 	 * uses lock_kernel()
 	 */
  	lock_kernel();
-	ispipe = format_corename(corename, retval, signr);
+	ispipe = format_corename(corename, signr);
 	unlock_kernel();
 	/*
 	 * Don't bother to check the RLIMIT_CORE value if core_pattern points

Re: [PATCH -mm] coredump: format_corename: don't append .%pid if multi-threaded

[Michael Kerrisk]

On Tue, Jul 22, 2008 at 2:18 PM, Oleg Nesterov <oleg@tv-sign.ru> wrote:
> If the coredumping is multi-threaded, format_corename() appends .%pid
> to the corename. This was needed before the proper multi-thread core
> dump support, now all the threads in the mm go into a single unified
> core file.
>
> Remove this special case, it is not even documented and we have "%p"
> and core_uses_pid.

Hi Oleg,

I have not thought about this at any length, but one question that
jumps to mind: could this feature still be useful for LinuxThreads,
where each thread does indeed have a separate PID?

Cheers,

Michael

>
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
>
> --- 26-rc2/fs/exec.c~FORMAT_CORENAME_NO_MT_PID  2008-07-22 15:42:15.000000000 +0400
> +++ 26-rc2/fs/exec.c    2008-07-22 15:46:04.000000000 +0400
> @@ -1373,7 +1373,7 @@ EXPORT_SYMBOL(set_binfmt);
>  * name into corename, which must have space for at least
>  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
>  */
> -static int format_corename(char *corename, int nr_threads, long signr)
> +static int format_corename(char *corename, long signr)
>  {
>        const char *pat_ptr = core_pattern;
>        int ispipe = (*pat_ptr == '|');
> @@ -1480,8 +1480,7 @@ static int format_corename(char *corenam
>         * If core_pattern does not include a %p (as is the default)
>         * and core_uses_pid is set, then .%pid will be appended to
>         * the filename. Do not do this for piped commands. */
> -       if (!ispipe && !pid_in_pattern
> -           && (core_uses_pid || nr_threads)) {
> +       if (!ispipe && !pid_in_pattern && core_uses_pid) {
>                rc = snprintf(out_ptr, out_end - out_ptr,
>                              ".%d", task_tgid_vnr(current));
>                if (rc > out_end - out_ptr)
> @@ -1745,7 +1744,7 @@ int do_coredump(long signr, int exit_cod
>         * uses lock_kernel()
>         */
>        lock_kernel();
> -       ispipe = format_corename(corename, retval, signr);
> +       ispipe = format_corename(corename, signr);
>        unlock_kernel();
>        /*
>         * Don't bother to check the RLIMIT_CORE value if core_pattern points
>
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [PATCH -mm] coredump: format_corename: don't append .%pid if multi-threaded

[Oleg Nesterov]

On 07/22, Michael Kerrisk wrote:
>
> On Tue, Jul 22, 2008 at 2:18 PM, Oleg Nesterov <oleg@tv-sign.ru> wrote:
> > If the coredumping is multi-threaded, format_corename() appends .%pid
> > to the corename. This was needed before the proper multi-thread core
> > dump support, now all the threads in the mm go into a single unified
> > core file.
> >
> > Remove this special case, it is not even documented and we have "%p"
> > and core_uses_pid.
> 
> Hi Oleg,
> 
> I have not thought about this at any length, but one question that
> jumps to mind: could this feature still be useful for LinuxThreads,
> where each thread does indeed have a separate PID?

As far as I know, LinuxThreads use CLONE_VM, right?

The coredump will create the single core file for all processes because
they have the same ->mm, the "threads" won't dump all over each other.

And, just in case, this patch doesn't make any difference if core_uses_pid
is set or pid_in_pattern is true.

That said, this is the user-visible change...

Oleg.

Re: [PATCH -mm] coredump: format_corename: don't append .%pid if multi-threaded

[Michael Kerrisk]

On Tue, Jul 22, 2008 at 2:43 PM, Oleg Nesterov <oleg@tv-sign.ru> wrote:
> On 07/22, Michael Kerrisk wrote:
>>
>> On Tue, Jul 22, 2008 at 2:18 PM, Oleg Nesterov <oleg@tv-sign.ru> wrote:
>> > If the coredumping is multi-threaded, format_corename() appends .%pid
>> > to the corename. This was needed before the proper multi-thread core
>> > dump support, now all the threads in the mm go into a single unified
>> > core file.
>> >
>> > Remove this special case, it is not even documented and we have "%p"
>> > and core_uses_pid.
>>
>> Hi Oleg,
>>
>> I have not thought about this at any length, but one question that
>> jumps to mind: could this feature still be useful for LinuxThreads,
>> where each thread does indeed have a separate PID?
>
> As far as I know, LinuxThreads use CLONE_VM, right?

Yes.

> The coredump will create the single core file for all processes because
> they have the same ->mm, the "threads" won't dump all over each other.

Yes, looks like you are right.  I had this vague idea that there were
circumstances where a dump of a LinuxThreads m-t process could produce
multipl core files, distinguished by the .PID, but I think I must have
misremembered.

> And, just in case, this patch doesn't make any difference if core_uses_pid
> is set or pid_in_pattern is true.
>
> That said, this is the user-visible change...

True.  Not sure how important that is in this case though.  What is
the reason for making this change (other than tidiness)?

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [PATCH -mm] coredump: format_corename: don't append .%pid if multi-threaded

[Oleg Nesterov]

On 07/22, Michael Kerrisk wrote:
>
> On Tue, Jul 22, 2008 at 2:43 PM, Oleg Nesterov <oleg@tv-sign.ru> wrote:
> >
> > That said, this is the user-visible change...
>
> True.  Not sure how important that is in this case though.  What is
> the reason for making this change (other than tidiness)?

Tidiness is the only reason.

Please don't hesitate to nack this patch if you think we shouldn't
change the historical behaviour, the cleanup is very minor.

As for me, I think it is a bit strange we append "%.pid" depending on
is_multithreaded, the same app can have 1 or more threads for various
reasons when the coredump happens, but this behaviour is very old and
perhaps it is too late to change.

Oleg.

Re: [PATCH -mm] coredump: format_corename: don't append .%pid if multi-threaded

[Michael Kerrisk]

On Tue, Jul 22, 2008 at 3:02 PM, Oleg Nesterov <oleg@tv-sign.ru> wrote:
> On 07/22, Michael Kerrisk wrote:
>>
>> On Tue, Jul 22, 2008 at 2:43 PM, Oleg Nesterov <oleg@tv-sign.ru> wrote:
>> >
>> > That said, this is the user-visible change...
>>
>> True.  Not sure how important that is in this case though.  What is
>> the reason for making this change (other than tidiness)?
>
> Tidiness is the only reason.
>
> Please don't hesitate to nack this patch if you think we shouldn't
> change the historical behaviour, the cleanup is very minor.

It is hard to think of something that might break because of this, so
I'll remain silent.

> As for me, I think it is a bit strange we append "%.pid" depending on
> is_multithreaded, the same app can have 1 or more threads for various

Agreed.  It is strange.

Cheers,

Michael

> reasons when the coredump happens, but this behaviour is very old and
> perhaps it is too late to change.
>
> Oleg.
>
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html