[Crash-utility] About displaying virtual memory information of exiting task

[qiaonuohan]

On 12/11/2014 12:34 AM, Dave Anderson wrote:
>
>
> ----- Original Message -----
>> Hello Dave,
>>
>> The patch has been modified, using pc->cmd_cleanup to clean tc->mm_struct.
>> Please check the attached patch.
>
> Hello Qiao,
>
> One major point, and a few minor ones...
>
> First, this won't work with the sample vmcore I showed you:
>
>    +static int
>    +is_valid_mm(ulong mm)
>    +{
>    +       char kbuf[BUFSIZE];
>    +       char *p;
>    +       int mm_users;
>    +
>    +       if (!(p = vaddr_to_kmem_cache(mm, kbuf, VERBOSE)))
>    +               goto bailout;
>    +
>    +       if (!STRNEQ(p, "mm_struct"))
>    +               goto bailout;
>    +
>    +       readmem(mm + OFFSET(mm_struct_mm_users), KVADDR,&mm_users, sizeof(int),
>    +               "mm_struct mm_users", FAULT_ON_ERROR);
>    +
>    +       return mm_users;
>    +
>    +bailout:
>    +       error(FATAL, "invalid mm_struct address\n");
>    +       return 0;
>    +}
>
> If you recall my sample vmcore, the mm->users was zero, but because
> the mm->count was non-zero, mmdrop() had not called __mmdrop() to
> free the mm_struct:
>
>      crash>  mm_struct.mm_count,owner,mm_users ffff880495120dc0
>      mm_count = {
>        counter = 2
>      }
>      owner = 0xffff88049863f500
>      mm_users = {
>        counter = 0
>      }
>    crash>
>
> So it should it test mm->count instead, correct?

Fixed.

>
> And maybe the error message above should indicate "invalid or stale", given
> that the user-supplied address may have been valid earlier?

Add an error message("stale mm_struct address") when mm_count equals to 0.

>
> Secondly, wouldn't it make more sense to just pass pc->curcmd_private to
> is_valid_mm() below, and if it fails, return NULL?  If it's an invalid
> argument, it doesn't make much sense to make the switch and set-up the
> call to pc->cmd_cleanup():

Yes. Valid check should go first.

>
>    -        if (!tm->mm_struct_addr)
>    -                return (ulong)NULL;
>    +        if (!tm->mm_struct_addr) {
>    +               if (pc->curcmd_flags&  MM_STRUCT_FORCE) {
>    +                       tc->mm_struct = tm->mm_struct_addr = pc->curcmd_private;
>    +
>    +                       /*
>    +                        * tc->mm_struct may be changed, use vm_cleanup to
>    +                        * restore it.
>    +                        */
>    +                       pc->cmd_cleanup_arg = (void *)task;
>    +                       pc->cmd_cleanup = vm_cleanup;
>    +
>    +                       if (!is_valid_mm(tm->mm_struct_addr))
>    +                               return (ulong)NULL;
>    +               } else
>    +                       return (ulong)NULL;
>    +       }
>
>
> And lastly, this works, but is somewhat of an overkill:
>
>    +static void
>    +vm_cleanup(void *arg)
>    +{
>    +       ulong task;
>    +       struct task_context *tc;
>    +       ulong mm_struct;
>    +
>    +       pc->cmd_cleanup = NULL;
>    +       pc->cmd_cleanup_arg = NULL;
>    +
>    +       task = (ulong)arg;
>    +
>    +       fill_task_struct(task);
>    +       mm_struct = ULONG(tt->task_struct + OFFSET(task_struct_mm));
>    +
>    +       tc = task_to_context(task);
>    +       tc->mm_struct = mm_struct;
>    +}
>
> The only way the tc->mm_struct swap is made is if tc->mm_struct was
> originally set to NULL.  So it could simply be:
>
>            tc = task_to_context(task);
>            tc->mm_struct = NULL;
> Right?

I failed to realize tm->mm_struct_addr comes from tc->mm_struct firstly, and
if tm->mm_struct_addr == 0, then tc->mm_struct must equal to 0. Thanks for
pointing it out.

I made a more change, passing tc to vm_cleanup, then task_to_context() will also
be omitted.

>
> Thanks,
>    Dave
> .
>


-- 
Regards
Qiao Nuohan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vm-M.patch
Type: text/x-patch
Size: 3941 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20141211/b63fe45d/attachment.bin>