This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: Patch [3/3] Userspace probes single stepping out-of-line
- From: Prasanna S Panchamukhi <prasanna at in dot ibm dot com>
- To: "Zhang, Yanmin" <yanmin dot zhang at intel dot com>
- Cc: systemtap at sources dot redhat dot com, "Keshavamurthy, Anil S" <anil dot s dot keshavamurthy at intel dot com>, "Mao, Bibo" <bibo dot mao at intel dot com>
- Date: Mon, 30 Jan 2006 14:15:28 +0530
- Subject: Re: Patch [3/3] Userspace probes single stepping out-of-line
- References: <99FA2ED298A9834DB1BF5DE8BDBF24130FCB11@pdsmsx403>
- Reply-to: prasanna at in dot ibm dot com
Yanmin,
Please see my comments inline below.
> >>7. Accessing user space pages not present in memory, from the
> >>registered callback routines.
> The patch uses the page_addr aligned with stack pointer to store instructions for single step.
> It doesn't consider scenarios of multi-thread process. For example, 1 process has 10 threads
> and every thread has an 8kb stack.
Initially this patch checks if there is enough free space in the current stack
page below %esp before storing the instructions for single stepping. I think this
should work even for multi threaded processes.
All the stacks share the same vma. Just near the end of
> the first 4kb page, threads might try to extend the same vma at the same time while every
> thread still has a stack page available. I suggest to use stack_addr - sizeof(long long) - size,
> if the result is bigger than vma->vm_start at VM_GROWDOWN case.
If there is no free space in the current stack page, we can check
for some space before vma->vm_start, and then expand beyond vm_start
if there is no space before vma->vm_start as you suggested.
We can synchronize among multiple-threads using mmap_sem, I will try and
implement this in the next set.
> >>+static int __kprobes copy_insn_onexpstack(struct uprobe *uprobe ,
> >>+ struct pt_regs *regs, struct vm_area_struct *vma)
> >>+{
> >>+ unsigned long addr, *vaddr, vm_addr;
> >>+ int err = 0, size = MAX_INSN_SIZE * sizeof(kprobe_opcode_t);
> >>+ struct vm_area_struct *new_vma;
> >>+ struct uprobe_page *upage;
> >>+ struct mm_struct *mm = current->mm;
> >>+ struct page *page;
> >>+ pte_t *pte;
> >>+
> >>+ upage = per_cpu_ptr(uprobe_page, smp_processor_id());
> Upage is per cpu data. If just jumping back to user space to execute the single step,
> device interrupt might disrupt the current thread, then current thread might be schedule
> to another processor. When the thread is woken up again, the upage is not the original.
> How about changing it to task_struct related?
yes, this is a good suggestion.
> >>+ if (vma->vm_flags & VM_GROWSDOWN)
> >>+ vm_addr = vma->vm_start - size;
> >>+ else if (vma->vm_flags & VM_GROWSUP)
> >>+ vm_addr = vma->vm_end + size;
> >>+ else
> >>+ return -EFAULT;
> >>+
> >>+ spin_lock(&mm->page_table_lock);
> >>+
> >>+ /* TODO: do we need to expand stack if extend_vma fails? */
> >>+ if (!(new_vma = find_extend_vma(mm, vm_addr)))
> >>+ err = expand_stack(new_vma, vm_addr);
> If new_vma==NULL, panic?
It should be just vma and not new_vma in the expand_stack call,
I will fix it.
> >>+ if (((stack_addr - sizeof(long long)) - page_addr) < size)
> If stack_addr==page_addr, above is always false because they are unsigned long. Pls. change it to:
> if ((stack_addr - sizeof(long long)) < (page_addr + size))
ok.
> >>+ return -ENOMEM;
> >>+
> >>+ if (__copy_to_user_inatomic((unsigned long *)(page_addr + size),
> Should (page_addr + size) is just page_addr?
yes, it should be page_addr.
> >>+ } else if (vma->vm_flags & VM_GROWSUP) {
> >>+ page_addr = (stack_addr & PAGE_MASK) + PAGE_SIZE;
> It's weird to get the page align by this approach. Not accurate.
Could you please suggest appropriate approach for this?
> >>+ if ((page_addr - (stack_addr + sizeof(long long))) < size)
> Similar unsigned long calculation issue.
This will be taken care in the next patch release.
Thanks for your comments.
Prasanna
--
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Email: prasanna@in.ibm.com
Ph: 91-80-51776329