Received-SPF: pass (google.com: domain of linux-kernel+bounces-79325-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 23 Feb 2024 15:50:33 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Dave Hansen <dave.hansen@intel.com>, John Groves <John@groves.net>, "Dan
 Williams" <dan.j.williams@intel.com>
CC: John Groves <jgroves@micron.com>, Jonathan Corbet <corbet@lwn.net>,
	"Vishal Verma" <vishal.l.verma@intel.com>, Dave Jiang <dave.jiang@intel.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>, Christian Brauner
	<brauner@kernel.org>, Jan Kara <jack@suse.cz>, Matthew Wilcox
	<willy@infradead.org>, <linux-cxl@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <nvdimm@lists.linux.dev>,
	<john@jagalactic.com>, Dave Chinner <david@fromorbit.com>, Christoph Hellwig
	<hch@infradead.org>, <dave.hansen@linux.intel.com>,
	<gregory.price@memverge.com>
Subject: Re: [RFC PATCH 16/20] famfs: Add fault counters
Message-ID: <65d92f49ee454_1711029468@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <cover.1708709155.git.john@groves.net>
 <43245b463f00506016b8c39c0252faf62bd73e35.1708709155.git.john@groves.net>
 <05a12c0b-e3e3-4549-b02e-442e4b48a86d@intel.com>
 <l66vdkefx4ut73jis52wvn4j6hzj5omvrtpsoda6gbl27d4uwg@yolm6jx4yitn>
 <65d8fa6736a18_2509b29410@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <ytyzwnrpxrc4pakw763qytiz2uft66qynwbjqhuuxrs376xiik@iazam6xcqbhv>
 <b26fc2d6-207c-4d93-b9a3-1fa81fd89f6c@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <b26fc2d6-207c-4d93-b9a3-1fa81fd89f6c@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?9fvIdi1KTJfDN4fauKntNWGrGO3kXIMQf5KVMyf9L5QU1jQ0j4ZbNTKZsYzw?=
 =?us-ascii?Q?yufNmMK6enq88pXOsMtA9gSvPaklWzd8mpKw+0fPAVR2iaIopkYuLD7e2OJA?=
 =?us-ascii?Q?6xocNcLSlWu1Uo+nwaVflrFyK7w3D5/duZhypwI/PkbZxhuTEk72eGPfibMh?=
 =?us-ascii?Q?bAoBKNQj6ETwRgyz/8us3Lk09bAgmlLe+tn/WJBGu+f5+pb+2e540R9cfGB8?=
 =?us-ascii?Q?JNdDWlGDujUDlabwVztao6FFibpKQQImkXhKOoV87+AoxkLbXohmF+TsnVQ/?=
 =?us-ascii?Q?DYwfUp9XqXmbvp84AGbCx+F1aU6qSgyCoIj97nbYldmqYD0talECs0zsTQuL?=
 =?us-ascii?Q?z3hR0cgZNL8W8V4rYOtx8KsNmDHa8Gbu+0rBIQMEXjfGWvXNC2MbgUUJRWOf?=
 =?us-ascii?Q?7ou4DhHiovu5nsNmQnpcB3D6y5a4qCmAWdpk8YvxvhuwsdzbbNcUsRtW9cB2?=
 =?us-ascii?Q?vlRyxSTpyjxfgikj8jIzhyZnhPZDROcxsTzie234Cxb47kzFg3AP6ku6w43N?=
 =?us-ascii?Q?Ii3vDBsZhSaWV73P5DV8/Mhl2ZSlSuLSkPpkdQfBJOiK+cmNae51kczum7w6?=
 =?us-ascii?Q?5r5LsKUto7622I/BfdCniMzLSghAv2nKwtflHRiuoD4Dq0EsfB6nJtO3tsad?=
 =?us-ascii?Q?bFTUa5+Qznp7HNNtNMUnlbQBhAH4emsZCi77Ubs9OzF/tUF7kRVATdCcvpNb?=
 =?us-ascii?Q?2SNRNV/NsKfIRmpuZW97EZT1aACEUQ98tz9EL0fuEypYu8nGBkyhlfQwxXJ2?=
 =?us-ascii?Q?yr4NgIgCaT/qIOawmrzfZyzwfiWR7tTHqhGAz4sKTNZMtbokVE/0tW7/YvhR?=
 =?us-ascii?Q?8UWlm0axHO/xEaSBXgPMQvwpBF9jUbo9G/AGPCTtIHFoUobCXIVgC7UtJUjc?=
 =?us-ascii?Q?AhTbxybDCUuxuEoFUhQKV7ZMO0reWy8pWkw33fR6rnHTGsQc9aLdxFUBY3f8?=
 =?us-ascii?Q?Q9reTqzp2Xw8vrCnauQHoowVXlJtc0z+FugcrM+z3q9fbae1kgbvRyALyng0?=
 =?us-ascii?Q?2+AFNa7hjAstUifvZMexAtMUxCTZi/B0c4NBWmYGweTCW0K3u3TNyhXMI4HE?=
 =?us-ascii?Q?WiKJ1wZxeTDFtWKl+eLphWD7onil5s01J2ziWE0fHG700ikQE753uTRPeY1S?=
 =?us-ascii?Q?oxUWxs6FhzKFkFiF9fWFBCdar+R+/MZm+/2dB/lkZuWFZ/rsYJWmLJExaWMG?=
 =?us-ascii?Q?5tQ7zjfz/b3Jzn4BuGwuUEkeNq+2YQ7AAhdNSZRJ8AaVjtJxPeZV6OBjaVla?=
 =?us-ascii?Q?WAFGNlfjkpE1q5/2/6ExAE7fquDtiCJY2vQTVOWi+6J/NPbsEzDS7Z1UMYi9?=
 =?us-ascii?Q?7Em8+c4MEcF5i9pyGPUZhRfKp0a27zqPZWwxJ87shnmLjBgYOUcrgQJENx3m?=
 =?us-ascii?Q?3dbqtnOsZHgArWj+oEXN0Nz1LvuLo00GnRbzywo0Qyh1c2WA0OLBkbWcvR+M?=
 =?us-ascii?Q?PTGveCAEz/0hbUhbrlQv3poh9aAVIiVg2diEVmDJq+4TkbuBoZ0md37JoMuz?=
 =?us-ascii?Q?MzyH0ZNwLxSxLFfVwUTrhNAUHL2U4tzhlejdlkTbS4AC/X7HcRoykjgLUPdU?=
 =?us-ascii?Q?z4hwDhoAPPclCFPt7QX2QpKMHauTha3MglSIFD1+mIpFCfpwuM93WYOYVp8z?=
 =?us-ascii?Q?zg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 359be0a6-8901-47ed-f844-08dc34ca3b9e
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Feb 2024 23:50:37.2198
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nXSePbOczYmwghFMe9p1EP8BAOvnJXwZlePlUMFO56hPMxF1+v/SrovSeqqm4lczPRHMq7cMzX48xVPAnZI94vh5I14To6NqyF8AuR1xRjo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4950
X-OriginatorOrg: intel.com

Dave Hansen wrote:
> On 2/23/24 12:39, John Groves wrote:
> >> We had similar unit test regression concerns with fsdax where some
> >> upstream change silently broke PMD faults. The solution there was trace
> >> points in the fault handlers and a basic test that knows apriori that it
> >> *should* be triggering a certain number of huge faults:
> >>
> >> https://github.com/pmem/ndctl/blob/main/test/dax.sh#L31
> > Good approach, thanks Dan! My working assumption is that we'll be able to make
> > that approach work in the famfs tests. So the fault counters should go away
> > in the next version.
> 
> I do really suspect there's something more generic that should be done
> here.  Maybe we need a generic 'huge_faults' perf event to pair up with
> the good ol' faults that we already have:
> 
> # perf stat -e faults /bin/ls
> 
>  Performance counter stats for '/bin/ls':
> 
>                104      faults
> 
> 
>        0.001499862 seconds time elapsed
> 
>        0.001490000 seconds user
>        0.000000000 seconds sys

Certainly something like that would have satisified this sanity test use
case. I will note that mm_account_fault() would need some help to figure
out the size of the page table entry that got installed. Maybe
extensions to vm_fault_reason to add VM_FAULT_P*D? That compliments
VM_FAULT_FALLBACK to indicate whether, for example, the fallback went
from PUD to PMD, or all the way back to PTE.

Then use cases like this could just add a dynamic probe in
mm_account_fault(). No real need for a new tracepoint unless there was a
use case for this outside of regression testing fault handlers, right?