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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5268CC54EE9 for ; Tue, 20 Sep 2022 02:44:04 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D515A8000D; Mon, 19 Sep 2022 22:44:03 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D012A80007; Mon, 19 Sep 2022 22:44:03 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BCB448000D; Mon, 19 Sep 2022 22:44:03 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id ADC6C80007 for ; Mon, 19 Sep 2022 22:44:03 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 8FCC4A0264 for ; Tue, 20 Sep 2022 02:44:03 +0000 (UTC) X-FDA: 79930919166.07.58401CE Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by imf23.hostedemail.com (Postfix) with ESMTP id F3BDC140015 for ; Tue, 20 Sep 2022 02:44:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1663641842; x=1695177842; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=pU0Lh7kPQjohzD7HoE3eq6jIAiz7ocbl3JHoRUJ+X4o=; b=c3YDjrkzLw0ahe/V6lGCvIURlJtDv0qP3kdmf3q60oTa1vPbhdlekSfu amBvSFkBCMxEhaVufMbX/r7b4M0CzdJI1N2eIo3A3mtYjvLPFVNXis4fT bFRzf8EvP3MJ3kHs3eH6LhKj/0KKAIicE5lweQRqm0izE58xUg4Q2XepM CqAwL6Juw8O0oe6yZUKnQ5lCn6Ik2CHOMyopSw4AG45s+gYdp5e0exu+i JzZj7aUd0hMHjd4frQAznDllW9FDye5HLn6qN4uff1inmDIyfrDdrVKpw gMLGFuWS6A8tprBKvL1fWldfI2rH8BpYLYgYxhNAoNH9duclI7oAdvkW8 Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10475"; a="300953965" X-IronPort-AV: E=Sophos;i="5.93,329,1654585200"; d="scan'208";a="300953965" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Sep 2022 19:44:00 -0700 X-IronPort-AV: E=Sophos;i="5.93,329,1654585200"; d="scan'208";a="794090364" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Sep 2022 19:43:57 -0700 From: "Huang, Ying" To: Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Baolin Wang , Zi Yan , Yang Shi , Oscar Salvador Subject: Re: [PATCH -V3 0/8] migrate_pages(): fix several bugs in error path References: <20220817081408.513338-1-ying.huang@intel.com> <20220817084833.b348d11eab362b2ac5a02259@linux-foundation.org> Date: Tue, 20 Sep 2022 10:43:45 +0800 In-Reply-To: <20220817084833.b348d11eab362b2ac5a02259@linux-foundation.org> (Andrew Morton's message of "Wed, 17 Aug 2022 08:48:33 -0700") Message-ID: <87k05ypxy6.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1663641842; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ST8ZmQtyOnO9k73qXRXo/FfwFYVWxf+Kd9RIAAHz+54=; b=M15QblAUTF/tB+KVJifgAzxMZPb2tHPnFY6kMmUyT8K4ktELDENWrk2yF94+hOwVXZtooL Va0rPQSJ1Ux8S0r9nB8pkhJyhC40qcJv5/uF+tozFfjYWpvbH5UZVIxIEBEKD7I9w9S0Jd /ff71WpLYBuXRjGCkC2QTL8NKYPim8g= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=c3YDjrkz; spf=pass (imf23.hostedemail.com: domain of ying.huang@intel.com designates 134.134.136.65 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1663641842; a=rsa-sha256; cv=none; b=fOVrbCID/GSqECgBH70cWp0K8P03f4fhsoy/0/kxNicNhlYSsbxbNlj13lZIXR0DrUrqYA waAPs1zLd9h5sRAqCTX58XHBm85zc3dVupUeZQGa17YUfc+Xi8PurCvzZoEzo9cDJiorPR fLFwHpfOwPMzJAvtAtjoF4kYCG1Ww8o= X-Rspam-User: Authentication-Results: imf23.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=c3YDjrkz; spf=pass (imf23.hostedemail.com: domain of ying.huang@intel.com designates 134.134.136.65 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com X-Rspamd-Server: rspam04 X-Stat-Signature: 61dkaa5twmp8q8zqiixw9ai1h95hmrtx X-Rspamd-Queue-Id: F3BDC140015 X-HE-Tag: 1663641841-963653 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Andrew Morton writes: > On Wed, 17 Aug 2022 16:14:00 +0800 Huang Ying wrote: > >> error-inject.patch, test-migrate.c, and test-migrate.sh are as below. >> It turns out that error injection is an important tool to fix bugs in >> error path. > > Indeed, and thanks for doing this. > > Did you consider lib/*inject*.c? If so, was it unsuitable? I have done some experiments to use some existing error injection mechanisms in kernel to test the error path of migrate_pages(). After some googling, I found that the BPF based error injection described in the following URL is most suitable for my purpose. https://lwn.net/Articles/740146/ Because the BPF seems quite flexible to satisfy various requirements of error injection. With it, the execution of some functions can be skipped and some specified error code can be returned instead. Works out of box ================ Some error injection functionality just works out of box. For example, inject some page allocation error in some path. Firstly, CONFIG_BPF_KPROBE_OVERRIDE needs to be enabled in kernel config. Then, a simple bpftrace script as follows can be used to inject page allocation error during migrate_pages(). --------------------ENOMEM----------------------- kprobe:migrate_pages { @in_migrate_pages++; } kretprobe:migrate_pages { @in_migrate_pages--; } kprobe:should_fail_alloc_page / @in_migrate_pages > 0 / { override(1); } ------------------------------------------------- The call chain of error injection is specified via the first 2 lines. I copied the methods used in BCC inject script. Is there any better method to specify the call chain? We can inject error only for THP page allocation too. --------------------ENOMEM THP------------------- kprobe:migrate_pages { @in_migrate_pages++; } kretprobe:migrate_pages { @in_migrate_pages--; } kprobe:should_fail_alloc_page / @in_migrate_pages > 0 && arg1 == 9 / { override(1); } ------------------------------------------------- Use some hack to override any function ====================================== The in-kernel BPF based error injection mechanism can only override function return value for the functions in the whitelist, that is, functions marked with ALLOW_ERROR_INJECTION(). That is quite limited. The thorough error path testing needs to override the return value of arbitrary function. So, I use a simple hack patch as follows for that. -----------------------8<--------------------------------- >From 3bcd401a3731bc7316d222501070a2a71fdf7170 Mon Sep 17 00:00:00 2001 From: Huang Ying Date: Tue, 20 Sep 2022 09:08:25 +0800 Subject: [PATCH] dbg: allow override any function with bpf_error_injection --- lib/error-inject.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/error-inject.c b/lib/error-inject.c index 1afca1b1cdea..82a402e0f15c 100644 --- a/lib/error-inject.c +++ b/lib/error-inject.c @@ -21,6 +21,7 @@ struct ei_entry { void *priv; }; +#if 0 bool within_error_injection_list(unsigned long addr) { struct ei_entry *ent; @@ -36,6 +37,12 @@ bool within_error_injection_list(unsigned long addr) mutex_unlock(&ei_mutex); return ret; } +#else +bool within_error_injection_list(unsigned long addr) +{ + return true; +} +#endif int get_injectable_error_type(unsigned long addr) { -- 2.35.1 ---------------------------------------------------------- With this debug patch, most error path can be tested. For example, --------------------ENOSYS THP + EAGAIN---------- #include kprobe:migrate_pages { @in_migrate_pages++; } kretprobe:migrate_pages { @in_migrate_pages--; } kprobe:unmap_and_move / @in_migrate_pages > 0 / { if (((struct page *)arg3)->flags & (1 << PG_head)) { override(-38); } else { override(-11); } } ------------------------------------------------- With this, unmap_and_move() will return -ENOSYS (-38) for THP, and -EAGAIN (-11) for normal page. This can be used to test the corresponding error path in migrate_pages(). I think that it's quite common for developers to inject error for arbitrary function to test the error path. Is it a good idea to turn on the arbitrary error injection if a special kernel configuration (e.g. CONFIG_BPF_KPROBE_OVERRIDE_ANY_FUNCTION) is enabled for debugging purpose only? Some hacks are still necessary for complete coverage ==================================================== Even if we can override the return value of any function. Some hacks are still necessary for complete coverage. For example, some functions may be inlined, if we want to override its return value, we need to mark it with "noinline". And some error cannot be injected with return value overridden directly. For example, if we want to test when THP split isn't allowed condition in migrate_pages(). Then, some hack patch need to be used to do that. For example, the below patch can do that. -----------------------8<--------------------------------- >From ca371806dc7f96148cbdf03fdf8f92309306a325 Mon Sep 17 00:00:00 2001 From: Huang Ying Date: Tue, 20 Sep 2022 09:37:53 +0800 Subject: [PATCH] dbg: inject nosplit --- mm/migrate.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/mm/migrate.c b/mm/migrate.c index 571d8c9fd5bc..d4ee76c285b2 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -57,6 +57,11 @@ #include "internal.h" +static noinline bool error_inject_nosplit(void) +{ + return false; +} + int isolate_movable_page(struct page *page, isolate_mode_t mode) { const struct movable_operations *mops; @@ -1412,6 +1417,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, bool nosplit = (reason == MR_NUMA_MISPLACED); bool no_subpage_counting = false; + if (error_inject_nosplit()) + nosplit = true; + trace_mm_migrate_pages_start(mode, reason); thp_subpage_migration: -- 2.35.1 ---------------------------------------------------------- With the help of the above patch, the following bpftrace script can inject the expected error, --------------------ENOMEM THP + nosplit--------- kprobe:migrate_pages { @in_migrate_pages++; } kretprobe:migrate_pages { @in_migrate_pages--; } kprobe:should_fail_alloc_page / @in_migrate_pages > 0 && arg1 == 9 / { override(1); } kprobe:error_inject_nosplit / @in_migrate_pages > 0 / { override(1); } ------------------------------------------------- Although some hack patches are needed. This is still simpler than my original hand-made error injection solution. So I will recommend developers to use it in the error path testing in the future. Best Regards, Huang, Ying