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: Review patches of user space kprobe


>>-----Original Message-----
>>From: Prasanna S Panchamukhi [mailto:prasanna@in.ibm.com]
>>Sent: 2006年1月5日 18:31
>>To: Zhang, Yanmin
>>Cc: systemtap@sources.redhat.com; Keshavamurthy, Anil S; Mao, Bibo
>>Subject: Re: Review patches of user space kprobe
>>
>>Yanmin,
>>
>>Thanks for your comments, Please see my comments inline below.
>>
>>>>>		/*inode of the application */
>>>>>		struct inode *inode;
>>>How about using a pointer to uprobe_module to locate the inode? Although
>>>the storage space is same, it's better to keep all the same info of
>>>uprobes in uprobe_module. Uprobes just need keep its own specific info.
>>
>>inode and offset combination uniquely identifies individual user space
>>probe,
What does "individual user space probe" mean here? Any uprobe will be shared by all processes who map the same page. Right?


 hence when a probe is hit, combination inode and offset is used to
>>identify each probe. Keeping inode pointer in struct uprobe clearly
>>reflects this concept.
What's the relationship between uprobe and uprobe_module? All uprobes linked by uprobe_module->ulist_head have the same inode, right? If it's correct, why not to just include inode in uprobe_module?


>>
>>
>>>>		/*offset within the page */
>>>This comment is incorrect. Functions, such like
>>>register_userspace_probe, use it as the offset from the beginning of the
>>>file, not a page of the file. Right? Although your example uses it as an
>>>offset of page. Frank's reply said this item could be deleted. I don't
>>>think he is correct because uprobe->kp.addr is used when calling
>>>register_kprobe.
>>
>>Next patch release will take care of this, offset is hidden from the user and user need
>>not initialize it.
>>
>>
>>>>		unsigned long offset;=20
>>>>		/*page containing probes */
>>>>		struct page *page;
>>>>		/* virtual memory area containing the probes */
>>>>		struct vm_area_struct *vma;
>>>Item page and vma are used temporarily. They are not used when the probe
>>>is hit. To shrink struct uprobe, they could be deleted. A new super
>>>struct could be defined to include struct uprobe, or always to search
>>>them when they are needed. Application might be inserted huge uprobes.
>>>It's better to keep the struct smaller.
>>
>>If vma's are temporarily defined, then readpage hooks should walk
>>through the vma list to get the vma. To reduce the time spent in
>>readpage/readpages hooks in fastpath, we store it while registeration.
Incorrect. This vma only represents one process who maps the page. How about other processes who map the same page?


>>
>>
>>
>>>>	};
>>>>
>>>>	struct uprobe_module {
>>>>		struct hlist_head ulist_head;
>>>>		/* hlist head containing list of probes within this
>>module */=20
>>>>		struct list_head mlist;
>>>>		/* list of uprobes_modules */
>>>>		struct inode *inode;
>>>Item inode could be deleted. Use uprobe_module->nd.dentry->d_inode.
>>
>>This will be taken care in the next patch release.
>>
>>
>>>>		/* inode of the application on which probes are
>>implanted */
>>>>		struct nameidata nd;
>>>>		/* to hold path/dentry etc. */
>>>Struct nameidata looks a little big. A pointer to dentry might be
>>>enough.
>>
>>nameidata is allocated per module/application and not for every probe.
My above questions answer this one.


>>At present the reference count to the mount point is held until
>>the probes is unregistered. I will relook at this issue.
>>
>>>>+	if (!kernel_text_address((unsigned long)p->addr))
>>>>+		addr =3D insert_kprobe_user(p);
>>>After calling insert_kprobe_user, p->addr is still equal to the return
>>>value of kmap_atomic. So when the uprobe is hit, how to find it from the
>>>hash table?
>>
>>Here it does not return just the mapped address, but inode * offset as the address.
>>Each probe is uniquely identified by the combination of inode and offset, kprobe_handler
>>does the same thing.
>>
>>>And I didn't see kprobe_handler is changed to call
>>>get_kprobe_user if uprobe is hit.
>>
>>kprobe_handler() calls get_uprobe_at() when the probe is hit.
>>
>>>>+ */
>>>>+static int map_uprobe_page(struct uprobe *up, int mapped)
>>>The second parameter seems useless.
>>
>>Its being used in the readpage patch.
>>
>>
>>
>>>>+{
>>>>+{
>>>>+	up->kp.addr =3D (kprobe_opcode_t *) ((unsigned long)up->kp.addr +
>>>>+				 (unsigned long)(up->offset &
>>~PAGE_MASK));
>>Does it guarantee later register_kprobe could distinguish it's a user
>>probe? Perhaps it's better to assign a fixed address in user space, such
>>like 0.
>>
>>Yes,register_kprobe() distinguishes user space probes. I could not
>>understand what you mean by "assign a fixed address in user space"
map_uprobe_page sets uprobe->kp.addr to address returned by kmap_atomic, then insert_probe_page will add offset to uprobe->kp.addr. Later, register_kprobe uses uprobe->kp.addr to estimate if the address is in kernel text. Is it wrong? As for why you didn't hit it, I think mostly now uprobe->kp.addr is pointing to the kernel data area, but it doesn't point to user space. Right? 
A fix address means a value out of kernel space. Such like a value under PAGE_OFFSET on i386.



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