From: Masami Hiramatsu <mhiramat@kernel.org> To: Ravi Bangoria <ravi.bangoria@linux.ibm.com> Cc: oleg@redhat.com, peterz@infradead.org, srikar@linux.vnet.ibm.com, rostedt@goodmis.org, acme@kernel.org, ananth@linux.vnet.ibm.com, akpm@linux-foundation.org, alexander.shishkin@linux.intel.com, alexis.berlemont@gmail.com, corbet@lwn.net, dan.j.williams@intel.com, jolsa@redhat.com, kan.liang@intel.com, kjlx@templeofstupid.com, kstewart@linuxfoundation.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, milian.wolff@kdab.com, mingo@redhat.com, namhyung@kernel.org, naveen.n.rao@linux.vnet.ibm.com, pc@us.ibm.com, tglx@linutronix.de, yao.jin@linux.intel.com, fengguang.wu@intel.com, jglisse@redhat.com Subject: Re: [PATCH v3 6/9] trace_uprobe: Support SDT markers having reference count (semaphore) Date: Tue, 8 May 2018 00:56:51 +0900 [thread overview] Message-ID: <20180508005651.45553d3cf72521481d16b801@kernel.org> (raw) In-Reply-To: <f3d066d2-a85a-bd21-d4f9-fc27e59135df@linux.ibm.com> On Mon, 7 May 2018 13:51:21 +0530 Ravi Bangoria <ravi.bangoria@linux.ibm.com> wrote: > Hi Masami, > > On 05/04/2018 07:51 PM, Ravi Bangoria wrote: > > > >>> +} > >>> + > >>> +static void sdt_increment_ref_ctr(struct trace_uprobe *tu) > >>> +{ > >>> + struct uprobe_map_info *info; > >>> + > >>> + uprobe_down_write_dup_mmap(); > >>> + info = uprobe_build_map_info(tu->inode->i_mapping, > >>> + tu->ref_ctr_offset, false); > >>> + if (IS_ERR(info)) > >>> + goto out; > >>> + > >>> + while (info) { > >>> + down_write(&info->mm->mmap_sem); > >>> + > >>> + if (sdt_find_vma(tu, info->mm, info->vaddr)) > >>> + sdt_update_ref_ctr(info->mm, info->vaddr, 1); > >> Don't you have to handle the error to map pages here? > > Correct.. I think, I've to feedback error code to probe_event_{enable|disable} > > and handler failure there. > > I looked at this. Actually, It looks difficult to feedback errors to > probe_event_{enable|disable}, esp. in the mmap() case. Hmm, can't you roll that back if sdt_increment_ref_ctr() fails? If so, how does sdt_decrement_ref_ctr() work in that case? > Is it fine if we just warn sdt_update_ref_ctr() failures in dmesg? I'm > doing this in [PATCH 7]. (Though, it makes more sense to do that in > [PATCH 6], will change it in next version). Of course we need to warn it at least, but the best is rejecting to enable it. > > Any better ideas? > > BTW, same issue exists for normal uprobe. If uprobe_mmap() fails, > there is no feedback to trace_uprobe and no warnigns in dmesg as > well !! There was a patch by Naveen to warn such failures in dmesg > but that didn't go in: https://lkml.org/lkml/2017/9/22/155 Oops, that's a real bug. It seems the ball is in Naveen's hand. Naveen, could you update it according to Oleg's comment, and resend it? > > Also, I'll add a check in sdt_update_ref_ctr() to make sure reference > counter never goes to negative incase increment fails but decrement > succeeds. OTOH, if increment succeeds but decrement fails, the > counter remains >0 but there is no harm as such, except we will > execute some unnecessary code. I see. Please carefully clarify whether such case is kernel's bug or not. I would like to know what the condition causes that uneven behavior. Thank you, > > Thanks, > Ravi > -- Masami Hiramatsu <mhiramat@kernel.org>
WARNING: multiple messages have this Message-ID (diff)
From: Masami Hiramatsu <mhiramat@kernel.org> To: Ravi Bangoria <ravi.bangoria@linux.ibm.com> Cc: oleg@redhat.com, peterz@infradead.org, srikar@linux.vnet.ibm.com, rostedt@goodmis.org, acme@kernel.org, ananth@linux.vnet.ibm.com, akpm@linux-foundation.org, alexander.shishkin@linux.intel.com, alexis.berlemont@gmail.com, corbet@lwn.net, dan.j.williams@intel.com, jolsa@redhat.com, kan.liang@intel.com, kjlx@templeofstupid.com, kstewart@linuxfoundation.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, milian.wolff@kdab.com, mingo@redhat.com, namhyung@kernel.org, naveen.n.rao@linux.vnet.ibm.com, pc@us.ibm.com, tglx@linutronix.de, yao.jin@linux.intel.com, fengguang.wu@intel.com, jglisse@redhat.com Subject: Re: [PATCH v3 6/9] trace_uprobe: Support SDT markers having reference count (semaphore) Date: Tue, 8 May 2018 00:56:51 +0900 [thread overview] Message-ID: <20180508005651.45553d3cf72521481d16b801@kernel.org> (raw) In-Reply-To: <f3d066d2-a85a-bd21-d4f9-fc27e59135df@linux.ibm.com> On Mon, 7 May 2018 13:51:21 +0530 Ravi Bangoria <ravi.bangoria@linux.ibm.com> wrote: > Hi Masami, > > On 05/04/2018 07:51 PM, Ravi Bangoria wrote: > > > >>> +} > >>> + > >>> +static void sdt_increment_ref_ctr(struct trace_uprobe *tu) > >>> +{ > >>> + struct uprobe_map_info *info; > >>> + > >>> + uprobe_down_write_dup_mmap(); > >>> + info = uprobe_build_map_info(tu->inode->i_mapping, > >>> + tu->ref_ctr_offset, false); > >>> + if (IS_ERR(info)) > >>> + goto out; > >>> + > >>> + while (info) { > >>> + down_write(&info->mm->mmap_sem); > >>> + > >>> + if (sdt_find_vma(tu, info->mm, info->vaddr)) > >>> + sdt_update_ref_ctr(info->mm, info->vaddr, 1); > >> Don't you have to handle the error to map pages here? > > Correct.. I think, I've to feedback error code to probe_event_{enable|disable} > > and handler failure there. > > I looked at this. Actually, It looks difficult to feedback errors to > probe_event_{enable|disable}, esp. in the mmap() case. Hmm, can't you roll that back if sdt_increment_ref_ctr() fails? If so, how does sdt_decrement_ref_ctr() work in that case? > Is it fine if we just warn sdt_update_ref_ctr() failures in dmesg? I'm > doing this in [PATCH 7]. (Though, it makes more sense to do that in > [PATCH 6], will change it in next version). Of course we need to warn it at least, but the best is rejecting to enable it. > > Any better ideas? > > BTW, same issue exists for normal uprobe. If uprobe_mmap() fails, > there is no feedback to trace_uprobe and no warnigns in dmesg as > well !! There was a patch by Naveen to warn such failures in dmesg > but that didn't go in: https://lkml.org/lkml/2017/9/22/155 Oops, that's a real bug. It seems the ball is in Naveen's hand. Naveen, could you update it according to Oleg's comment, and resend it? > > Also, I'll add a check in sdt_update_ref_ctr() to make sure reference > counter never goes to negative incase increment fails but decrement > succeeds. OTOH, if increment succeeds but decrement fails, the > counter remains >0 but there is no harm as such, except we will > execute some unnecessary code. I see. Please carefully clarify whether such case is kernel's bug or not. I would like to know what the condition causes that uneven behavior. Thank you, > > Thanks, > Ravi > -- Masami Hiramatsu <mhiramat@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-05-07 15:56 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-17 4:32 [PATCH v3 0/9] trace_uprobe: Support SDT markers having reference count (semaphore) Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` [PATCH v3 1/9] Uprobe: Export vaddr <-> offset conversion functions Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` [PATCH v3 2/9] mm: Prefix vma_ to vaddr_to_offset() and offset_to_vaddr() Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-05-03 14:59 ` Steven Rostedt 2018-05-03 14:59 ` Steven Rostedt 2018-04-17 4:32 ` [PATCH v3 3/9] Uprobe: Move mmput() into free_map_info() Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` [PATCH v3 4/9] Uprobe: Rename map_info to uprobe_map_info Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` [PATCH v3 5/9] Uprobe: Export uprobe_map_info along with uprobe_{build/free}_map_info() Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` [PATCH v3 6/9] trace_uprobe: Support SDT markers having reference count (semaphore) Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-05-04 4:48 ` Masami Hiramatsu 2018-05-04 4:48 ` Masami Hiramatsu 2018-05-04 14:21 ` Ravi Bangoria 2018-05-04 14:21 ` Ravi Bangoria 2018-05-07 8:21 ` Ravi Bangoria 2018-05-07 8:21 ` Ravi Bangoria 2018-05-07 15:56 ` Masami Hiramatsu [this message] 2018-05-07 15:56 ` Masami Hiramatsu 2018-05-08 9:46 ` Naveen N. Rao 2018-05-08 9:46 ` Naveen N. Rao 2018-05-08 10:26 ` Ravi Bangoria 2018-05-08 10:26 ` Ravi Bangoria 2018-05-24 16:26 ` Oleg Nesterov 2018-05-24 16:26 ` Oleg Nesterov 2018-05-25 8:28 ` Ravi Bangoria 2018-05-25 8:28 ` Ravi Bangoria 2018-04-17 4:32 ` [PATCH v3 7/9] trace_uprobe/sdt: Fix multiple update of same reference counter Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-04-17 4:32 ` [PATCH v3 8/9] trace_uprobe/sdt: Document about " Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-05-03 14:57 ` Masami Hiramatsu 2018-05-03 14:57 ` Masami Hiramatsu 2018-04-17 4:32 ` [PATCH v3 9/9] perf probe: Support SDT markers having reference counter (semaphore) Ravi Bangoria 2018-04-17 4:32 ` Ravi Bangoria 2018-05-03 14:57 ` Masami Hiramatsu 2018-05-03 14:57 ` Masami Hiramatsu 2018-05-03 8:32 ` [PATCH v3 0/9] trace_uprobe: Support SDT markers having reference count (semaphore) Ravi Bangoria 2018-05-03 8:32 ` Ravi Bangoria 2018-05-04 14:30 ` Oleg Nesterov 2018-05-04 14:30 ` Oleg Nesterov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180508005651.45553d3cf72521481d16b801@kernel.org \ --to=mhiramat@kernel.org \ --cc=acme@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=alexander.shishkin@linux.intel.com \ --cc=alexis.berlemont@gmail.com \ --cc=ananth@linux.vnet.ibm.com \ --cc=corbet@lwn.net \ --cc=dan.j.williams@intel.com \ --cc=fengguang.wu@intel.com \ --cc=jglisse@redhat.com \ --cc=jolsa@redhat.com \ --cc=kan.liang@intel.com \ --cc=kjlx@templeofstupid.com \ --cc=kstewart@linuxfoundation.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=milian.wolff@kdab.com \ --cc=mingo@redhat.com \ --cc=namhyung@kernel.org \ --cc=naveen.n.rao@linux.vnet.ibm.com \ --cc=oleg@redhat.com \ --cc=pc@us.ibm.com \ --cc=peterz@infradead.org \ --cc=ravi.bangoria@linux.ibm.com \ --cc=rostedt@goodmis.org \ --cc=srikar@linux.vnet.ibm.com \ --cc=tglx@linutronix.de \ --cc=yao.jin@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.