From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
To: Song Liu <songliubraving@fb.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org, matthew.wilcox@oracle.com,
kernel-team@fb.com, william.kucharski@oracle.com,
kirill.shutemov@linux.intel.com, Oleg Nesterov <oleg@redhat.com>
Subject: Re: [PATCH v2 5/5] uprobe: only do FOLL_SPLIT_PMD for uprobe register
Date: Fri, 18 Oct 2019 18:22:23 +0530 [thread overview]
Message-ID: <20191018104349.GB29201@linux.vnet.ibm.com> (raw)
In-Reply-To: <20191017164223.2762148-6-songliubraving@fb.com>
* Song Liu <songliubraving@fb.com> [2019-10-17 09:42:22]:
> Attaching uprobe to text section in THP splits the PMD mapped page table
> into PTE mapped entries. On uprobe detach, we would like to regroup PMD
> mapped page table entry to regain performance benefit of THP.
>
> However, the regroup is broken For perf_event based trace_uprobe. This is
> because perf_event based trace_uprobe calls uprobe_unregister twice on
> close: first in TRACE_REG_PERF_CLOSE, then in TRACE_REG_PERF_UNREGISTER.
> The second call will split the PMD mapped page table entry, which is not
> the desired behavior.
>
> Fix this by only use FOLL_SPLIT_PMD for uprobe register case.
>
> Add a WARN() to confirm uprobe unregister never work on huge pages, and
> abort the operation when this WARN() triggers.
>
> Fixes: 5a52c9df62b4 ("uprobe: use FOLL_SPLIT_PMD instead of FOLL_SPLIT")
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
Looks good to me.
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> kernel/events/uprobes.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 94d38a39d72e..c74761004ee5 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -474,14 +474,17 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
> struct vm_area_struct *vma;
> int ret, is_register, ref_ctr_updated = 0;
> bool orig_page_huge = false;
> + unsigned int gup_flags = FOLL_FORCE;
>
> is_register = is_swbp_insn(&opcode);
> uprobe = container_of(auprobe, struct uprobe, arch);
>
> retry:
> + if (is_register)
> + gup_flags |= FOLL_SPLIT_PMD;
> /* Read the page with vaddr into memory */
> - ret = get_user_pages_remote(NULL, mm, vaddr, 1,
> - FOLL_FORCE | FOLL_SPLIT_PMD, &old_page, &vma, NULL);
> + ret = get_user_pages_remote(NULL, mm, vaddr, 1, gup_flags,
> + &old_page, &vma, NULL);
> if (ret <= 0)
> return ret;
>
> @@ -489,6 +492,12 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
> if (ret <= 0)
> goto put_old;
>
> + if (WARN(!is_register && PageCompound(old_page),
> + "uprobe unregister should never work on compound page\n")) {
> + ret = -EINVAL;
> + goto put_old;
> + }
> +
> /* We are going to replace instruction, update ref_ctr. */
> if (!ref_ctr_updated && uprobe->ref_ctr_offset) {
> ret = update_ref_ctr(uprobe, mm, is_register ? 1 : -1);
> --
> 2.17.1
>
--
Thanks and Regards
Srikar Dronamraju
prev parent reply other threads:[~2019-10-18 12:52 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-17 16:42 [PATCH v2 0/5] Fixes for THP in page cache Song Liu
2019-10-17 16:42 ` [PATCH v2 1/5] proc/meminfo: fix output alignment Song Liu
2019-10-17 17:37 ` Yang Shi
2019-10-17 16:42 ` [PATCH v2 2/5] mm/thp: fix node page state in split_huge_page_to_list() Song Liu
2019-10-17 17:38 ` Yang Shi
2019-10-17 16:42 ` [PATCH v2 3/5] mm: Support removing arbitrary sized pages from mapping Song Liu
2019-10-17 17:43 ` Yang Shi
2019-10-17 16:42 ` [PATCH v2 4/5] mm/thp: allow drop THP from page cache Song Liu
2019-10-17 21:46 ` Yang Shi
2019-10-18 13:32 ` Kirill A. Shutemov
2019-10-18 21:54 ` Yang Shi
2019-10-17 16:42 ` [PATCH v2 5/5] uprobe: only do FOLL_SPLIT_PMD for uprobe register Song Liu
2019-10-18 12:52 ` Srikar Dronamraju [this message]
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=20191018104349.GB29201@linux.vnet.ibm.com \
--to=srikar@linux.vnet.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=kernel-team@fb.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=matthew.wilcox@oracle.com \
--cc=oleg@redhat.com \
--cc=songliubraving@fb.com \
--cc=william.kucharski@oracle.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).