Re: About displaying virtual memory information of exiting task

[Dave Anderson <anderson@xxxxxxxxxx>]

----- 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?

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

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():
  
  -        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?

Thanks,
  Dave

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility