From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4FD30154BEA for ; Thu, 26 Dec 2024 09:04:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735203868; cv=none; b=GSyK8L0wWsx0N7VvHAVEv5zh6Erxxl6FdSccFxZVWWcLszs+xBQhtpnVsqOzjo4KkS+NnRE8QVHtlOXOzn+GTKNHBWV5LtXdOaDxsL8PyDv7KmehjX5US9OgwfKblvA7OqJU6STzEuaWhLorUU1Yt81tCWmz8JcEYIrhNbOEWrY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735203868; c=relaxed/simple; bh=zcSCy5IEhPo4izAQkjpZl0PDag3eVhrytTuA1E/L938=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=qxfYSDhcFAM5xj2KTRlIcMSp/2jCkxT+g0diPhj58D1UKnHj/sXGIGyUrYwhkjFqVgO3ju7nEuqj9QMl7qmh2c66kdDLKU+5ytwsQUcVrgA0d8w11EZMBdXfJYxpKhUxF2uYBT7PaZtzD0UpIUauhnXQWXOlt9vdB+EvN3SEcm8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=grPBwVV7; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="grPBwVV7" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1735203865; h=from:from: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=zcSCy5IEhPo4izAQkjpZl0PDag3eVhrytTuA1E/L938=; b=grPBwVV7/jfvAGlzATA/H77+AgAAFLUJ/oGbMSHYYfW87uV1qbs/ZJBgtFv/GkakX49iuz 3LUaLCxQWxg+kSvLuI2wRQhrSjLmZUXbO4rCH+M4ZWTtM4WtNBYA1AXHf5Sxv3Y41DRODw qtzJxLDHO1zn7DlttXi+sxDAT43pbAg= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-304-96RagDo5Nza89EiL6tLQAA-1; Thu, 26 Dec 2024 04:04:23 -0500 X-MC-Unique: 96RagDo5Nza89EiL6tLQAA-1 X-Mimecast-MFC-AGG-ID: 96RagDo5Nza89EiL6tLQAA Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-4361ecebc5bso36376195e9.1 for ; Thu, 26 Dec 2024 01:04:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735203862; x=1735808662; h=mime-version:user-agent:content-transfer-encoding:autocrypt :references:in-reply-to:date:cc:to:from:subject:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=zcSCy5IEhPo4izAQkjpZl0PDag3eVhrytTuA1E/L938=; b=ZWVdcbJ0MOoQxLFr1n5WuychZRkAcKkFU1J7/aIys+0fSCukBtzdXUXZjs7tBpA8sM bO/rNzNSwIe17nOK9o6OJMXGwRL3n1nXkpHe3ESo5SLaPZn2dyiy6HTBL6MuLHPbMyPh lnHljpPzd9hdspyTRSXB44b4zyEwDULvyt0WVWUhQwfp+uR+QYCqbwFJtI/zwuIEFBGY 8MH6OmE4JynQSL04WgGgHcRYC06Pf+Nvn0UL8BiF9I0YErXEgOJo+4jz7mCXJBXxkOHd +rPOQaTSvm+KtYZAKYCXmWNSup3N0xzrkXDdtpRjGmY5M2pHE+ErBWVz9Hh+m2V1J02S OGVA== X-Forwarded-Encrypted: i=1; AJvYcCUlQXeEC2jIEDLfM9jafT1uSRPXfgn7O8VtesT0CI6SzOKqQw5sJDmovhyjU0J1OfGrLZdJD8AtVm69kl0=@vger.kernel.org X-Gm-Message-State: AOJu0YzkDlLljdFF1UaN6g9PTq6sosRx/YGo9Prt0sIILIm7PBdUOEZv 2pDNKZusuvxAphp+U9lhISY0BtMu5Lr4KMvS/qQ+sir6LczEowuxIAPwoUVAjg0tyRDNYcDPaMu jyDZ6TTooy3gLu3F+0hyadIbpuPVWO7KTpujcJdD12xxxYSRXBfNlr8coJ2vaOw== X-Gm-Gg: ASbGnct7bvwWZzeE0cmLx5+at7okjEdcz5Ytd88qq3v8O6mCblvCCm3Znmkyb7YtKYU olA7IJgI/r/pKPEmhUtsCfjpTr9zCbeD/osdT5c7t1CjAZb1y/jZ5H+Bsi4+KIkfSevHelSP/gU YAMAVZ407NrrTxRN6jcCVHvdepmCt9FUNYwBhxClqU/yrHJxH8W7862iG2bDxeWkQncAOCGn+7z 5ze+IBy/rbssJRWORSJOKF1s/9hzPOleTXf1MfTQOQcBGb5WilGC0BxeF7JHD+U3ReiBe8LZdaE hTmWToIVUg== X-Received: by 2002:a05:600c:4509:b0:436:1c04:aa8e with SMTP id 5b1f17b1804b1-4366864676emr199566515e9.16.1735203862371; Thu, 26 Dec 2024 01:04:22 -0800 (PST) X-Google-Smtp-Source: AGHT+IHjvxlaI/4f32BW7JJYstlUZCLypZbNhrZnM3gA1+pwoTO5ztebx1wpyLWDfO860QmndZRGdA== X-Received: by 2002:a05:600c:4509:b0:436:1c04:aa8e with SMTP id 5b1f17b1804b1-4366864676emr199566165e9.16.1735203861915; Thu, 26 Dec 2024 01:04:21 -0800 (PST) Received: from gmonaco-thinkpadt14gen3.rmtit.csb ([195.174.134.200]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38a1c832e69sm18824058f8f.35.2024.12.26.01.04.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Dec 2024 01:04:21 -0800 (PST) Message-ID: <83fa755bad5e607cf242cacccb58a4ea2490b8a0.camel@redhat.com> Subject: Re: [PATCH v3 3/3] rseq/selftests: Add test for mm_cid compaction From: Gabriele Monaco To: Mathieu Desnoyers , Peter Zijlstra , Ingo Molnar , linux-mm@kvack.org, linux-kernel@vger.kernel.org Cc: Juri Lelli , Shuah Khan Date: Thu, 26 Dec 2024 10:04:20 +0100 In-Reply-To: <6c159869-8f01-4aa5-9df1-7a0d6e3c23b7@efficios.com> References: <20241216130909.240042-1-gmonaco@redhat.com> <20241216130909.240042-4-gmonaco@redhat.com> <6c159869-8f01-4aa5-9df1-7a0d6e3c23b7@efficios.com> Autocrypt: addr=gmonaco@redhat.com; prefer-encrypt=mutual; keydata=mDMEZuK5YxYJKwYBBAHaRw8BAQdAmJ3dM9Sz6/Hodu33Qrf8QH2bNeNbOikqYtxWFLVm0 1a0JEdhYnJpZWxlIE1vbmFjbyA8Z21vbmFjb0ByZWRoYXQuY29tPoiZBBMWCgBBFiEEysoR+AuB3R Zwp6j270psSVh4TfIFAmbiuWMCGwMFCQWjmoAFCwkIBwICIgIGFQoJCAsCBBYCAwECHgcCF4AACgk Q70psSVh4TfJzZgD/TXjnqCyqaZH/Y2w+YVbvm93WX2eqBqiVZ6VEjTuGNs8A/iPrKbzdWC7AicnK xyhmqeUWOzFx5P43S1E1dhsrLWgP Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.54.2 (3.54.2-1.fc41) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Tue, 2024-12-24 at 11:20 -0500, Mathieu Desnoyers wrote: > On 2024-12-16 08:09, Gabriele Monaco wrote: > > A task in the kernel (task_mm_cid_work) runs somewhat periodically > > to > > compact the mm_cid for each process, this test tries to validate > > that > > it runs correctly and timely. > >=20 > > + if (curr_mm_cid =3D=3D 0) { > > + printf_verbose( > > + "mm_cids successfully compacted, exiting\n"); > > + pthread_exit(NULL); > > + } > > + usleep(RUNNER_PERIOD); > > + } > > + assert(false); >=20 > I suspect we'd want an explicit error message here > with an abort() rather than an assertion which can be > compiled-out with -DNDEBUG. >=20 > > + } > > + printf_verbose("cpu%d has %d and is going to terminate\n", > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sched_getcpu(), curr_mm_cid); > > + pthread_exit(NULL); > > +} > > + > > +void test_mm_cid_compaction(void) >=20 > This function should return its error to the caller > rather than assert. >=20 > > +{ > > + cpu_set_t affinity; > > + int i, j, ret, num_threads; > > + pthread_t *tinfo; > > + pthread_mutex_t *token; > > + struct thread_args *args; > > + > > + sched_getaffinity(0, sizeof(affinity), &affinity); > > + num_threads =3D CPU_COUNT(&affinity); > > + tinfo =3D calloc(num_threads, sizeof(*tinfo)); > > + if (!tinfo) { > > + fprintf(stderr, "Error: failed to allocate tinfo(%d): %s\n", > > + errno, strerror(errno)); > > + assert(ret =3D=3D 0); > > + } > > + args =3D calloc(num_threads, sizeof(*args)); > > + if (!args) { > > + fprintf(stderr, "Error: failed to allocate args(%d): %s\n", > > + errno, strerror(errno)); > > + assert(ret =3D=3D 0); > > + } > > + token =3D calloc(num_threads, sizeof(*token)); > > + if (!token) { > > + fprintf(stderr, "Error: failed to allocate token(%d): %s\n", > > + errno, strerror(errno)); > > + assert(ret =3D=3D 0); > > + } > > + if (num_threads =3D=3D 1) { > > + printf_verbose( > > + "Running on a single cpu, cannot test anything\n"); > > + return; >=20 > This should return a value telling the caller that > the test is skipped (not an error per se). >=20 Thanks for the review! I'm not sure how to properly handle these, but it seems to me the cleanest way is to use ksft_* functions to report failures and skipped tests. Other tests in rseq don't use the library but it doesn't seem a big deal if just one test is using it, for now. It gets a bit complicated to return values since we are exiting from the main thread (sure we could join the remaining /winning/ thread but we would end up with 2 threads running). The ksft_* functions solve this quite nicely using exit codes, though. > > + } > > + pthread_mutex_init(token, NULL); > > + /* The main thread runs on CPU0 */ > > + for (i =3D 0, j =3D 0; i < CPU_SETSIZE && j < num_threads; i++) { > > + if (CPU_ISSET(i, &affinity)) { >=20 > We can save an indent level here by moving this > in the for () condition: >=20 > =C2=A0for (i =3D 0, j =3D 0; i < CPU_SETSIZE && > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CPU_ISSET(i, &affinity) && j < num_threads= ; i++) { >=20 Well, if we assume the affinity mask is contiguous, which is likely but not always true. A typical setup with isolated CPUs have one housekeeping core per NUMA node, let's say 0,32,64,96 out of 127 cpus, the test would run only on cpu 0 in that case. > > + args[j].num_cpus =3D num_threads; > > + args[j].tinfo =3D tinfo; > > + args[j].token =3D token; > > + args[j].cpu =3D i; > > + args[j].args_head =3D args; > > + if (!j) { > > + /* The first thread is the main one */ > > + tinfo[0] =3D pthread_self(); > > + ++j; > > + continue; > > + } > > + ret =3D pthread_create(&tinfo[j], NULL, thread_runner, > > + =C2=A0=C2=A0=C2=A0=C2=A0 &args[j]); > > + if (ret) { > > + fprintf(stderr, > > + "Error: failed to create thread(%d): %s\n", > > + ret, strerror(ret)); > > + assert(ret =3D=3D 0); > > + } > > + ++j; > > + } > > + } > > + printf_verbose("Started %d threads\n", num_threads); >=20 > I think there is a missing rendez-vous point here. Assuming a > sufficiently long unexpected delay (think of a guest VM VCPU > preempted for a long time), the new leader can start poking > into args and other thread's info while we are still creating > threads here. >=20 Yeah, good point, I'm assuming all threads are ready by the time we are done waiting but that's not bulletproof. I'll add a barrier. Thanks again for the comments, I'll prepare a V4. Gabriele