From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_MUTT autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2DBDBC2BCA1 for ; Fri, 7 Jun 2019 20:14:11 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 64F91208C0 for ; Fri, 7 Jun 2019 20:14:10 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="G2TH77zw" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 64F91208C0 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 45LDJH3BC5zDr1x for ; Sat, 8 Jun 2019 06:14:07 +1000 (AEST) Authentication-Results: lists.ozlabs.org; spf=none (mailfrom) smtp.mailfrom=infradead.org (client-ip=2607:7c80:54:e::133; helo=bombadil.infradead.org; envelope-from=willy@infradead.org; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=infradead.org header.i=@infradead.org header.b="G2TH77zw"; dkim-atps=neutral Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2607:7c80:54:e::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 45LDG25cRhzDr0R for ; Sat, 8 Jun 2019 06:12:09 +1000 (AEST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=V+gSVnIv9QBKq1mZboB8NpBLa64Y8lrKysJJF3r5Di4=; b=G2TH77zwcRW+ghgSutS9pXWwx q06PFG90kLdW9nSZ8eT0zhsmJkCONTk9ihOiwCD/vOWRCH7+RiVotuRpO+PXMPtB8viY749wFWiHD e35WP8ojqL7BLHfJjBicKoWZKTv7dWog/hIuqMTYYTk2e38Bi6ed1zJgJ9LV8EZFY6S7kzQtNbxWR dq2EVJjEtvyg+7uncwjU/GfOave+g4SmAyk4QaIqVw/cBAp5JROl4fss5kOmhhd48m/zKN/BT2Kne 4r7aT25INlQYUt07MKB6e3JiljH56Bv5dF61FNPZ9GAePTzIrq6vFfaE5EIdtGfqYacXVUhZ0+nnV aWTdDN8VA==; Received: from willy by bombadil.infradead.org with local (Exim 4.92 #3 (Red Hat Linux)) id 1hZLDX-0002mb-5b; Fri, 07 Jun 2019 20:12:03 +0000 Date: Fri, 7 Jun 2019 13:12:03 -0700 From: Matthew Wilcox To: Anshuman Khandual Subject: Re: [RFC V3] mm: Generalize and rename notify_page_fault() as kprobe_page_fault() Message-ID: <20190607201202.GA32656@bombadil.infradead.org> References: <1559903655-5609-1-git-send-email-anshuman.khandual@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1559903655-5609-1-git-send-email-anshuman.khandual@arm.com> User-Agent: Mutt/1.9.2 (2017-12-15) X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Michal Hocko , linux-ia64@vger.kernel.org, linux-sh@vger.kernel.org, Peter Zijlstra , Catalin Marinas , Dave Hansen , Will Deacon , linux-mm@kvack.org, Paul Mackerras , sparclinux@vger.kernel.org, linux-s390@vger.kernel.org, Yoshinori Sato , x86@kernel.org, Russell King , Ingo Molnar , Fenghua Yu , Stephen Rothwell , Andrey Konovalov , Andy Lutomirski , Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Tony Luck , Heiko Carstens , linux-kernel@vger.kernel.org, Martin Schwidefsky , Andrew Morton , linuxppc-dev@lists.ozlabs.org, "David S. Miller" Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" Before: > @@ -46,23 +46,6 @@ kmmio_fault(struct pt_regs *regs, unsigned long addr) > return 0; > } > > -static nokprobe_inline int kprobes_fault(struct pt_regs *regs) > -{ > - if (!kprobes_built_in()) > - return 0; > - if (user_mode(regs)) > - return 0; > - /* > - * To be potentially processing a kprobe fault and to be allowed to call > - * kprobe_running(), we have to be non-preemptible. > - */ > - if (preemptible()) > - return 0; > - if (!kprobe_running()) > - return 0; > - return kprobe_fault_handler(regs, X86_TRAP_PF); > -} After: > +++ b/include/linux/kprobes.h > @@ -458,4 +458,20 @@ static inline bool is_kprobe_optinsn_slot(unsigned long addr) > } > #endif > > +static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs, > + unsigned int trap) > +{ > + int ret = 0; > + > + /* > + * To be potentially processing a kprobe fault and to be allowed > + * to call kprobe_running(), we have to be non-preemptible. > + */ > + if (kprobes_built_in() && !preemptible() && !user_mode(regs)) { > + if (kprobe_running() && kprobe_fault_handler(regs, trap)) > + ret = 1; > + } > + return ret; > +} Do you really think this is easier to read? Why not just move the x86 version to include/linux/kprobes.h, and replace the int with bool? On Fri, Jun 07, 2019 at 04:04:15PM +0530, Anshuman Khandual wrote: > Very similar definitions for notify_page_fault() are being used by multiple > architectures duplicating much of the same code. This attempts to unify all > of them into a generic implementation, rename it as kprobe_page_fault() and > then move it to a common header. I think this description suffers from having been written for v1 of this patch. It describes what you _did_, but it's not what this patch currently _is_. Why not something like: Architectures which support kprobes have very similar boilerplate around calling kprobe_fault_handler(). Use a helper function in kprobes.h to unify them, based on the x86 code. This changes the behaviour for other architectures when preemption is enabled. Previously, they would have disabled preemption while calling the kprobe handler. However, preemption would be disabled if this fault was due to a kprobe, so we know the fault was not due to a kprobe handler and can simply return failure. This behaviour was introduced in commit a980c0ef9f6d ("x86/kprobes: Refactor kprobes_fault() like kprobe_exceptions_notify()") > arch/arm/mm/fault.c | 24 +----------------------- > arch/arm64/mm/fault.c | 24 +----------------------- > arch/ia64/mm/fault.c | 24 +----------------------- > arch/powerpc/mm/fault.c | 23 ++--------------------- > arch/s390/mm/fault.c | 16 +--------------- > arch/sh/mm/fault.c | 18 ++---------------- > arch/sparc/mm/fault_64.c | 16 +--------------- > arch/x86/mm/fault.c | 21 ++------------------- > include/linux/kprobes.h | 16 ++++++++++++++++ What about arc and mips?