This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Patch [3/3] Userspace probes single stepping out-of-line


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]