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+/jVSAgAC9awCAAMgaAIAAqcoAgAAG4AA=
Date:   Wed, 19 Jul 2023 23:44:59 +0000
Message-ID: <E93923C4-080B-4B43-9A3B-28A233BF5DFC@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>
In-Reply-To: <168980881867.11078.6059884952065090216@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?rPhzUE5+aZg/QzWH0BGFtDQhiW4fU16QwOnksvvRVcvsB7SLc3Gl/sy+3wD0?=
 =?us-ascii?Q?A5CIjxo+2XrfTyMrfjxAxD8eEyTWF6GXv5DLn309aH9w1nZ2BSQH6QmaTd8z?=
 =?us-ascii?Q?z1FO0ubq3Z5uZ0+0vrBl1EpIy1pYhLY9SlCWo2to+MCo45G+BZ6ClR3fm8b2?=
 =?us-ascii?Q?xKtuFpT3w+gpeHRMDJtA7kJkbawsIp7UfbN6rPxzKvCMv9I65Awj2gqRQl8C?=
 =?us-ascii?Q?Re7+6dDY9tDk8Yg7toNXRPfbXRsDCEQUjioQR9jjLI9o3neZu4gBpr4ZlSqc?=
 =?us-ascii?Q?FHriJ9NXr4hrWT9xX4aDICLNJ4quLxjp/3Gk90bhaQ9e2I2/nqD3EmrfF8IU?=
 =?us-ascii?Q?kfMdSzwOYk0Ry1CNc79dArtbqj21nNbT55uqTTiFzo9QxHbLFsyIg0b4ZvkP?=
 =?us-ascii?Q?6NLhznA6IS9h0uymaPhFJSInHd9VWyTgVcnkbdfHs87zvuRT1FLEOHSOpvHV?=
 =?us-ascii?Q?qDl03yut5xrtuZJyFKaK7w7v7SoMqlXa3LCtic5ku2Iz8SIhQoNuwxJGaq0S?=
 =?us-ascii?Q?vLaocMcYqS2LH015WMZbO6bMj8zTiXhV/spuangJdL4/GnrCtbcudSIAVae8?=
 =?us-ascii?Q?34EvJw4dxtoZ5BwEXrdIiakl/niSlUDF7YrghFF5rZhCPdAcZSf+BpL26PlY?=
 =?us-ascii?Q?X8fgmotLqDq27kJVbXJZRzyJJwILGIY6tOux9n6mdAhjCeDpI5OhnLsldgHV?=
 =?us-ascii?Q?cXte/HLOfCpc4oUPVXdQo7qsSrwn2TV55dmQrt//SBWT6vHJg3cfEvIALJAa?=
 =?us-ascii?Q?GNce+GStEhRTO3eO3D/KkSh7XWEz9ODlJpK0PvQXAgRLTlagCpyVYEFPKRfT?=
 =?us-ascii?Q?Ya4cBnG8DMijW7Px6cgf/8fo1hWkNhc7WkgnqcQq6AM91ws9YFz8P/6yYkov?=
 =?us-ascii?Q?RiP+ApAbehqKHCftjNvGq2O2Uha/iiN7GpUrdLa3zRxfHpWT/BJYd1saBS/F?=
 =?us-ascii?Q?D0WQJg7fAUj9bHFh6RpLiGSEuqBIQlILDvIhR7JtTQ4JJYoywE05+SLPtlQS?=
 =?us-ascii?Q?DPxYM8WyL+HoFi4U7bO9Qh98AWyhLO+Wdm4cdRKkBFgKxa4Jsjdk5oETM0+7?=
 =?us-ascii?Q?FtqGjYpW1PSrlNRI3OgGbnh9uSxAw6fDl3PA8ebDhiX4r7VQ+w4xtogRbr3m?=
 =?us-ascii?Q?jeTkLcpypOUTZ4Gvm/2bMlSVF9nP750lUHk3tocQkSTUyExUl7C43quOeByr?=
 =?us-ascii?Q?h1BFH1rzwuwL0P5bRPG3BEjusnQWHlQoSn6tsRJJvaCQjWe/y6xgm3lt45nu?=
 =?us-ascii?Q?W97SNaUVWnxeONHaxgOBwrYuRXLGNIJ5NHTrwWT6CkDjQbcCvVE3jEIEVYWi?=
 =?us-ascii?Q?q0lDjRruHM2tfxTot8dNVh1tMhdOxzDKR0tiG6Z6rjuawqy3MtKlollud9OV?=
 =?us-ascii?Q?MPWniMvUguW3YPXfr6dRy+GdzneiQ8ZmVdcqPhu+0Fb9Q6MFbcx4Zjj0Zgy5?=
 =?us-ascii?Q?YEcOrh0x0n4X5Jmp6It8msfYjqST4sZ2JG1B7AYvwo5TQVbVB4rvswsgzW9j?=
 =?us-ascii?Q?efovIfw0ZcYmGPa7cQG8nqUh9n901vMTwrsFirEstskiPIlEFXVdOF0PWDis?=
 =?us-ascii?Q?r41RLEBADjmnLDKe3LdIp3w0F88FFY4QPJJb5Rb5frT6/un1JE1pleU4Vqlo?=
 =?us-ascii?Q?OQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <760C3D56A9E43E409ECE4CA86F6EC67C@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: NAzp+6XQRpCMvT+8D8MkS9rm3a40CWhhGi5l13utGHsxicIZ0JVaFFt5tiackY79lqMJkEyhR9xCLGh87O8usCfPjdQiizYzP61xXOa8e5LxgmAodRUtYxbazHsQ4VdJ7ESe+Orozzb/XDPl61VVw+qJLsHpQi3tIwf0FI8i/WM/TTp26iGPQJKvV//csZev6xI19eETQogMawjUTQ9VQJ5hS3/y+8N9uJqQW3XH+q5g1hIe9me9bsAwu1hE+/2cDDxUA1GIc5rSsaQw7Zt20MpWyLmVA3/TYTypX4+IY9FcFqXJW7Fcucd6DW+jULjG5hXcwQQEh0PWvs7bJ03D/9nB6vl/+Szd9qXr84jAH9EXOLIJn74+XQwZjY9K+W92Fzhm/K09ryoa9GhLhdYzwMaBCLWtRzhgzWX47hiTiT9GRbcJrBo5vvkB/mlCi/nUDz1aTh7xwXwNP7nzLKfL4Q0ruX93n3kwDkZ+T+b+s2rG7mmdhl2DoFqCB5VlhH6shEl0+adFqooTXbKqjO5kPA+SYSyasnj4kwpm+u/mlgIbrOVZknkdyn/vscOKC9bZ7HgwPd3st1wcZOlsahu5KAitSllPAdGv7yZ6Qhi6nIE9M3YVp/ir8mwa2kQBwmqJQSajroAIeHntJ7y+WzLGMSbgd15TCSOV/YlUY4F61LxIXp0+f8a7poo+BaJTdO2fKS4GR/URt9OVTjkfH+RXtLPUf7slI7waDldRHBjzSq2XJ6zv5VyRg90o0GtthJusrYxS9M/DCmzsGijliMW5haQziQEmVqZJWnYS+pSw/9fWh9NffAuh43xsbIxX2iax
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: 31704d54-7fb6-4586-4d10-08db88b22a15
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jul 2023 23:44:59.4355
 (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: p3lVQI9shCWDpBVERCPb2u+OYCbDIPXfYCZh5aep06FgakZELgXKrQWKKEN5UTl44YruDcvKCRGRqprmxQi+iA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB6676
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-19_16,2023-07-19_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 mlxlogscore=999
 suspectscore=0 phishscore=0 mlxscore=0 adultscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2306200000
 definitions=main-2307190214
X-Proofpoint-ORIG-GUID: CYfIXo7_IuAU1Nk9GUFVSOyWXYsoaUs6
X-Proofpoint-GUID: CYfIXo7_IuAU1Nk9GUFVSOyWXYsoaUs6
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: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 woken=
 -
>>>>> 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.

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.


> I think it is worth logging whether an event triggered a wake up or not,
> and which event did that.

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.


> 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


--
Chuck Lever