Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Neil Brown <neilb@suse.de>
CC:     Chuck Lever <cel@kernel.org>, Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 10/14] SUNRPC: change svc_pool_wake_idle_thread() to
 return nothing.
Thread-Topic: [PATCH 10/14] SUNRPC: change svc_pool_wake_idle_thread() to
 return nothing.
Thread-Index: AQHZuUKpaE3Yn0PdG0Ko1CF5yZZwz6+/jVSAgAC9awCAAMgaAIAAqcoAgAAG4ACAAPceAA==
Date:   Thu, 20 Jul 2023 14:29:33 +0000
Message-ID: <3A9F5306-EAEE-427C-80D2-E0CD81212238@oracle.com>
References: <168966227838.11075.2974227708495338626.stgit@noble.brown>
 <168966228866.11075.18415964365983945832.stgit@noble.brown>
 <ZLaagzqpB9MsQ5yb@bazille.1015granger.net>
 <168972938409.11078.8409356274248659649@noble.neil.brown.name>
 <9EEE82A6-6D25-4939-A4F5-BAC8E9986FF3@oracle.com>
 <168980881867.11078.6059884952065090216@noble.neil.brown.name>
 <E93923C4-080B-4B43-9A3B-28A233BF5DFC@oracle.com>
In-Reply-To: <E93923C4-080B-4B43-9A3B-28A233BF5DFC@oracle.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?X+0peeQoWuLHF9DJbhDt4lWZglOpxJT4Sc3hAK4X9r28pH4ueAi9RAQtJDZJ?=
 =?us-ascii?Q?MFz+e0o4euNNWVOTaDL8SSwEeJNoI8YUmnFsZ+p8tN0rcXhmlYvTgjn6pjfa?=
 =?us-ascii?Q?Zi4gkiAqfF0jnxCutU8Y2fut+igkidlGqolJN0q67vTlxqWbjIA0j7/0p7qL?=
 =?us-ascii?Q?xxFmDFzteIccH4HZplZD3pmhe2MU3T1aAEDHKBPT2N0fJuzk2wDQOFcROtP2?=
 =?us-ascii?Q?SNxavLze46W+1WoiAMsjUsIIGzLHo9szgrS/GY+8Zn9m1C8ybaB7lWBXUc9h?=
 =?us-ascii?Q?+ucNBtzQAZu4GB9iPK9g3rrBexRqWDaNgdq1RNOT/nMZ388DGP/kmFNcxzzl?=
 =?us-ascii?Q?dNDbRooo3yAQMyKu44OzExtXELxwVtFpzg3Q2/rO1zTm6bmU+MSSYqG4hp/P?=
 =?us-ascii?Q?8MCs6yF5DtIwyMYls6BXMECMZz+1FrDcs+0M6yPJrmQuDtFntyPbdg2ohi5R?=
 =?us-ascii?Q?QXSlMFdaIgRlxwWoUkKs/9yEFidf6FNjxPNQWpXSD94c9GczON788afMHlsW?=
 =?us-ascii?Q?QzPU4LfrlQPllX7/K3i0guQU3vgxT2gTv1rtMiNz8aQ06EMxeDagjkygvSN1?=
 =?us-ascii?Q?fLNsR2FOoZb1Uw9uvb7pdE5xltd15p3zub63z1SNS7jiyFjQq35NpbwLd34Q?=
 =?us-ascii?Q?jAYUCEIpYtWQa1R+UZlCbHProZC+6J6i3tMmD+RUjmsuh1s4AZOlURjTMqmh?=
 =?us-ascii?Q?McE1qIfN8ShCIVNC7XZm9tGjZ2pm/USmnio56I22OGZljp//RGkyMf6a+8Un?=
 =?us-ascii?Q?9I7feGWBI7WPM+wTfNStj6L16c50SUT0a5XCssA/JeXq0XY/GffG9ZJ/I/I0?=
 =?us-ascii?Q?6cXrQpr8cRb7HDR13SsZA/CuNCwbV6NdXticXpulYBOJRfqUpujYHdB0OBAp?=
 =?us-ascii?Q?yvitWlyD0BEJLcf5AUwpYb0cERNt1QF/y6TSr135m/36lHe23vWsTA2n4KCL?=
 =?us-ascii?Q?L/pD0tyKF76nuXNDbDx8UsJ2kewAzlNimbXLs8OHaI4UdRYyWPC0vqPzCZQH?=
 =?us-ascii?Q?PJ0U+NXef0oU2x6pKgwulzB3HWbhNFbTtXW/8w1HOc1/JnvRD/BJi68yLkSV?=
 =?us-ascii?Q?5pS6snZx+zRzqc8uNTX1o/FvaFYLJ8iwm7AQo9xeWRLwYg293rJBlNnwnFE6?=
 =?us-ascii?Q?M07UQ8hW2c1AUmWShw7bBc0R6ERXKRsMFZq2ub89j6YFopt+xZeUSgFtPJ74?=
 =?us-ascii?Q?3ef018OntoXHZ/w6guDEQl9thYI9pMbD8Z4VkdzifrKbOw/lK+4/GL1y/gEZ?=
 =?us-ascii?Q?2MZq/sNxDqAL8dXd3kPK6UPE2aHYt5t3ccTb6NfO7sXiUY6NA1pP+HuCS6E8?=
 =?us-ascii?Q?9NWv18/NhKNuncOZlIKgvYFM0Z87i9ljNaFJnzTmIsPrU4kL2VAChYxL5UhN?=
 =?us-ascii?Q?IwZTDmYVqW0ricm/KudhXwVr6Hr+XuaPc10P/QaNDHSuGAZN2XdsIvpDOY1e?=
 =?us-ascii?Q?cInpHdKBMt/fu41Jg4M/7nINRgU4oB41sZMeLL5ddGBxF0LIPY39cGmgl/yf?=
 =?us-ascii?Q?3VqslHirk9LLRpHMZXPEwUthJp7JHaQIHO+By8z2HK6SbXDyylEizspH0c5h?=
 =?us-ascii?Q?xAW5QASd/TUd1zLbfee28GKK0Lu9HWeHyYXXalgibdkqaz6mkRTbjPCFTMdV?=
 =?us-ascii?Q?iQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C628877E46336E4C9E82A25CF0B3E470@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 3KnTakd1hJuuDwqy4Dy1ydxf0m+7ge0ddwsWkK4/9c4JUoDXANlLI1vppokUjFj9BALj5iuqftkWDu4/9CyRuh28kwH2qGrCRkKxepMKen+OPWEktodSGptP2Sxv5TI1eY9FIUS+1UNW5nlu9bqDuEsVk4O2ZqWmVEgfkPI7wdUvfstCSCd5X1H2XMVD/YBU1/rLtJ2broGHn7fkJ2GmXIgi4VlkfRxZv1jbH5i0L0Ty9HAgK9Gi7QxFAivp4j3OiVs6nGE2DqP43gd26lxDO4q4SqkuZVIMyHm1SNHcyLwew71S4HvFSkBYIq7tVcHRr6WCZaG88J4/30iq9r3ktodHcWIRYpnpfwrxMeq4MI+beIursvHCKbB+IYf1hkRcFxiuDJxkht5AxL8qhxTIzkqcq8GhTHhkmvexG4/5jJ7Uoii4RdmFK+pgGbKRqIVyLGBoS+rhDKF790kDxm/QCwdwdyMr/A8i2aubzUQ8vixifh+U9v+LGm6opcAPEa8MG+UNv1ilmtlWhgWE/ST9I/ft4B9Cq9nk3/B3tE2nul7vZYWxr7yi9QnlCIFdT8xm8tcYX/E65smbZ+224pP5qh5ZD/vy+pMaeMtbvMImm7HEnra6zvNfDLlpQKYciU7qFFHHiBLV0TN+iRSuH/fAt0sxp9ScYrxMfBNRB800z/2uvJa+kv6kr8/InFAmMU89GCcNGrYJb8UbGpqyXz3dRLUpL+E3ADBYqAv2jciXEHtSuXi82P6mrtwTyezjDiACJ7cDidEMrC4IgeGnmW9XNQjPWoT+Je9MUSwvr6sWmeidwbY5X/jmmdHE4sh7TQh5
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6c9b366b-264c-4451-a4bc-08db892dbc75
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jul 2023 14:29:33.0852
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: H62vZJf6V3r7frnJyQx4PKteUpDlNQAETCYzR7GZL9E/sxxnqVQ275I225X7kDNWVXKyHuI8Ora44K4oSTXKnQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB4901
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-20_07,2023-07-20_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxlogscore=999
 suspectscore=0 phishscore=0 bulkscore=0 mlxscore=0 malwarescore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2306200000 definitions=main-2307200122
X-Proofpoint-GUID: k7gINPYtzZAoL1UjiTqpf5Ih5JNKAjcF
X-Proofpoint-ORIG-GUID: k7gINPYtzZAoL1UjiTqpf5Ih5JNKAjcF
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jul 19, 2023, at 7:44 PM, Chuck Lever III <chuck.lever@oracle.com> wro=
te:
>=20
>=20
>=20
>> On Jul 19, 2023, at 7:20 PM, NeilBrown <neilb@suse.de> wrote:
>>=20
>> On Wed, 19 Jul 2023, Chuck Lever III wrote:
>>>=20
>>>> On Jul 18, 2023, at 9:16 PM, NeilBrown <neilb@suse.de> wrote:
>>>>=20
>>>> On Tue, 18 Jul 2023, Chuck Lever wrote:
>>>>> On Tue, Jul 18, 2023 at 04:38:08PM +1000, NeilBrown wrote:
>>>>>> No callers of svc_pool_wake_idle_thread() care which thread was woke=
n -
>>>>>> except one that wants to trace the wakeup.  For now we drop that
>>>>>> tracepoint.
>>>>>=20
>>>>> That's an important tracepoint, IMO.
>>>>>=20
>>>>> It might be better to have svc_pool_wake_idle_thread() return void
>>>>> right from it's introduction, and move the tracepoint into that
>>>>> function. I can do that and respin if you agree.
>>>>=20
>>>> Mostly I agree.
>>>>=20
>>>> It isn't clear to me how you would handle trace_svc_xprt_enqueue(),
>>>> as there would be no code that can see both the trigger xprt, and the
>>>> woken rqst.
>>>>=20
>>>> I also wonder if having the trace point when the wake-up is requested
>>>> makes any sense, as there is no guarantee that thread with handle that
>>>> xprt.
>>>>=20
>>>> Maybe the trace point should report when the xprt is dequeued.  i.e.
>>>> maybe trace_svc_pool_awoken() should report the pid, and we could have
>>>> trace_svc_xprt_enqueue() only report the xprt, not the rqst.
>>>=20
>>> I'll come up with something that rearranges the tracepoints so that
>>> svc_pool_wake_idle_thread() can return void.
>>=20
>> My current draft code has svc_pool_wake_idle_thread() returning bool -
>> if it found something to wake up - purely for logging.
>=20
> This is also where I have ended up. I'll post an update probably tomorrow
> my time. Too much other stuff going on to finish it today.

Pushed to https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
in branch topic-sunrpc-thread-scheduling


>> I think it is worth logging whether an event triggered a wake up or not,
>> and which event did that.
>=20
> Agreed. I have some experimental code that records _RET_IP_ of the caller
> of svc_xprt_enqueue(), but again it's questionable whether that is of
> long term value.
>=20
>=20
>> I'm less you that the pid is relevant.  But
>> as you say - this will probably become clearer as the code settles down.
>>=20
>> Thanks,
>> NeilBrown
>>=20
>>=20
>>>=20
>>> svc_pool_wake_idle_thread() can save the waker's PID in svc_rqst
>>> somewhere, for example. The dequeue tracepoint can then report that
>>> (if it's still interesting when we're all done with this work).
>>>=20
>>>=20
>>> --
>>> Chuck Lever
>=20
>=20
> --
> Chuck Lever


--
Chuck Lever