Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <deaaabe5-0a0c-b6f9-c85b-5080874f6437@oracle.com>
Date:   Sun, 3 Jul 2022 21:09:05 +1000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.11.0
Subject: Re: [RESEND PATCH] kernfs: Avoid re-adding kernfs_node into
 kernfs_notify_list.
Content-Language: en-US
To:     Tejun Heo <tj@kernel.org>
Cc:     gregkh@linuxfoundation.org, viro@zeniv.linux.org.uk,
        m.szyprowski@samsung.com, nathan@kernel.org, michael@walle.cc,
        robh@kernel.org, linux-serial@vger.kernel.org,
        linux-kernel@vger.kernel.org, guillaume.tucker@collabora.com,
        pmladek@suse.com
References: <20220701154604.2211008-1-imran.f.khan@oracle.com>
 <Yr9U1q0BBinCgyrT@mtj.duckdns.org>
From:   Imran Khan <imran.f.khan@oracle.com>
In-Reply-To: <Yr9U1q0BBinCgyrT@mtj.duckdns.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bExuL0pUQmJmL3ZCZEhtYzRUVmJQVkc0RjlvbDNZd0RlWE5aZTZlYlFDeWRN?=
 =?utf-8?B?SWN0U1ZsVHF3UE9jQ0dweVU3Mkx0NjhsRVFURS80ajIxZ1UyS0FHcTdyNWM1?=
 =?utf-8?B?aXhCYUxaMFRjWS9ySkwwbXRCOGlCZllOTjVtRHdocVJERFI4cm9YVzM0Vmd3?=
 =?utf-8?B?S29hQWt4STI3N0toZzNlSHQ5Uk82dDJkNTIwMExoS1V2WmJsTWg4R1JMeFhX?=
 =?utf-8?B?dXZsZWFsTFBHTm5VRnp4eEpCU3lVWFFIWFM2WnRmU2ZpS2ttc2NNNTNGSVFQ?=
 =?utf-8?B?Nk1MWWlmK2wzcFg3TVd2ZDJpQW5iVVRlNm1BREo1cVZ4ejFVcVlxeGxWTyt5?=
 =?utf-8?B?bGtZR1JNYjdBN3FIbTF6dzhJL25BZlUycmdBeTREcFg2TU5MdWVRdUtkaDVI?=
 =?utf-8?B?NHBKVEhjVG0vVy9Cd09oUXJQcTU0THMxRC9zY010S2pTMzlycWtTR25lekcw?=
 =?utf-8?B?R2pHS1p4aVNzY281bk0vdHJDalFOb1psNjVwMFVrSEZTSjhnekJIUHlUMVB1?=
 =?utf-8?B?TTh5YlhLLzM4cHcrNHl0dndFV2ZYdC9rcEw0RENuaWlnNFhBejB2bzQwU0Nx?=
 =?utf-8?B?dCtCbnBtVHhYNjZRMHEyV3NrcDJCRVhkVGRVSitNam5zVEF1bEdvdGkvc0dZ?=
 =?utf-8?B?SllEUzVVcFV2clo0SjJlQ3JVQjRLRm01dXdYU3plOGtJYkYrekMxcnB5aWg4?=
 =?utf-8?B?cDBVN3FxYjRtei94WTJTdXpYOU9zUDdFY1NGcmJCSm5VVW41R0czcXAyMy9I?=
 =?utf-8?B?YkVveFY0VVlBMmIyT0RSdVFKMDR1aGw5ZGJqZitRWGVra21yMTBKRXdETFlJ?=
 =?utf-8?B?MXZYZWpoR3o2bHBDM1YxR21IWE40SS9iSFpEQmJFZmlQN2ZKdEs1QnFxRjhX?=
 =?utf-8?B?TTF5c2ppYy96OStzYUVybHdEbVAzWkxsRG92a0NuaEllS092dkIrVnVYRlVH?=
 =?utf-8?B?NGtaMjNac3RzR3lyRzNuR284T3R0YmVsNGJUekJ2VnlyRWVKaThZdXNXVTBU?=
 =?utf-8?B?RlpXbXIyWjdyRWtmcjZOUFZBOHJiU2ltNHJmVzlsRWY4VXM2NXl5U1NwUVZ4?=
 =?utf-8?B?bFh4ZExFUmd0cUE1ZmU3YnNwZUFYSE9LSk5XV2dXbnVNL1dEcTQrSXVuMEhH?=
 =?utf-8?B?OS95TC9EUXhTQlkvbTBkRWN3V3JwdHhBM25DZXJUQkwzblRrdmNNdlRuYXhO?=
 =?utf-8?B?RkkwS2haUUpVQ0R3d3JiUVpoT0I1V2xWNHN2UmtqaHBDVnhNdk0rWTFZeGlF?=
 =?utf-8?B?SXdBc1ZhYk01WS8vTGx2ZFhuN0VaWkZqeDZuMXExV0lrR1l2T3BkcWVFQjBq?=
 =?utf-8?B?UHdJNDlQakxzWnJaRm9mMkJNVWJHTm9acmE2VGQxR2p4L2xrbitqWGVjU2h5?=
 =?utf-8?B?c2M4U1QvY3BvSGlmZ1ZHbCtoalN6SmhSSDRVOWVnRWFaZGZEd1oxMjlScW9F?=
 =?utf-8?B?MnpCT294QUNBeVBKdjlMa3dSNDJXWVFicnBXSUt3VEJIaFlnQytoUGpNbG1B?=
 =?utf-8?B?NlR5UE53V002MjQrYU9VZVp2U2F5c3Q3RnhObGFHY3M2QVlrSmpKTllXak5X?=
 =?utf-8?B?elc3MlZaWXM3RWtORFN0bkVUaTlBb2VFS0s3eFFvT0RQaW1LU0VnTkJNZlJM?=
 =?utf-8?B?aDNZa21UZ3BwdkxvRS9xN1BaWU5Vb3dtWFQ0c1Fkanhjd004S0lqOERZNlEx?=
 =?utf-8?B?THQrNHJyV0pYZ0Z1TWJIdW54N0FZOEFLaUtpOFIrY1FWb1RFaGxJaWtCK1dJ?=
 =?utf-8?B?UlBsMlcxZGxTYUZIUmlFdWxUWGpSTFg1OVFQbytRV2ZIWjJ0bnU4dENCVHdP?=
 =?utf-8?B?elVUVnJnRzhUZW03bStMbVZtT2JVSEVxZzdmUWd2MEk1NzArQTdzNGJqckRz?=
 =?utf-8?B?WmxMTitsemkvM2xVS1Z2Z2lhYldZSjU4NXFmakgxZU45cUlXbHRpZTZWVGto?=
 =?utf-8?B?SmVtbzFZMFIxUFN4L0FNK0xCR3lvdnMxQ3l1VHNXQWlOMzh2ZHRtUS8rV1NN?=
 =?utf-8?B?VGZRbWNtS04xWDhTZjRhcHY1WE5hbzgvcndZWitHTmtpK2RTbDlFOVM3MGRS?=
 =?utf-8?B?cEdHQWY4c3ZpUThwTktSTTdoR0s2YWlkSnFnNCtRTHNRc2NkQ2ltcEhxYUMw?=
 =?utf-8?Q?c5xQMx54Ed/7lmd2S1aAxUoen?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6c2f7bcc-ddae-44d8-347f-08da5ce47adc
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4468.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jul 2022 11:09:21.3021
 (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: cQcNVYHqH9ykgjosMlc9y8yhty0y6m8HcyEzosZTqueW5sWnSqDW9E0Y8tXN0FdjDncx5sHMuzFVzUzNxJ4ADg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB3838
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-07-03_07:2022-06-28,2022-07-03 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 adultscore=0 suspectscore=0
 malwarescore=0 bulkscore=0 mlxlogscore=999 phishscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2204290000
 definitions=main-2207030051
X-Proofpoint-GUID: 2DlKlnjopHlb80DufvwHz-TXv1VTB_sR
X-Proofpoint-ORIG-GUID: 2DlKlnjopHlb80DufvwHz-TXv1VTB_sR
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,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-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello Tejun,
Thanks for your feedback.

On 2/7/22 6:11 am, Tejun Heo wrote:
> Hello,
> 
> On Sat, Jul 02, 2022 at 01:46:04AM +1000, Imran Khan wrote:
>> @@ -992,9 +993,11 @@ void kernfs_notify(struct kernfs_node *kn)
>>  	rcu_read_unlock();
>>  
>>  	/* schedule work to kick fsnotify */
>> -	kernfs_get(kn);
>> -	llist_add(&kn->attr.notify_next, &kernfs_notify_list);
>> -	schedule_work(&kernfs_notify_work);
>> +	if (kn->attr.notify_next.next != NULL) {
>> +		kernfs_get(kn);
>> +		llist_add(&kn->attr.notify_next, &kernfs_notify_list);
>> +		schedule_work(&kernfs_notify_work);
>> +	}
> 
> Aren't you just narrowing the race window here? What prevents two
> threads simultaneously testing for non NULL and then entering the
> addition block together?
> 
Indeed that is possible.
> Looked at the llist code and it doesn't support multiple producers
> trying to add the same node, unfortunately, so I'm not sure llist is
> gonna work here. For now, the right thing to do prolly is reverting
> it.
> 

Can we use kernfs_notify_lock like below snippet to serialize producers
(kernfs_notify):

spin_lock_irqsave(&kernfs_notify_lock, flags);
if (kn->attr.notify_next.next != NULL) {
	kernfs_get(kn);
	llist_add(&kn->attr.notify_next, &kernfs_notify_list);
	schedule_work(&kernfs_notify_work);
}
spin_unlock_irqsave(&kernfs_notify_lock, flags);

As per following comments at the beginning of llist.h

 * Cases where locking is not needed:
 * If there are multiple producers and multiple consumers, llist_add can be
 * used in producers and llist_del_all can be used in consumers simultaneously
 * without locking. Also a single consumer can use llist_del_first while
 * multiple producers simultaneously use llist_add, without any locking.

Multiple producers and single consumer can work in parallel but as in our case
addition is dependent on kn->attr.notify_next.next != NULL, we may keep the
checking and list addition under kernfs_notify_lock and for consumer just lock
free->next = NULL under kernfs_notify_lock.

Having said this, I am okay with reverting the llist change as well, because
anyways it is not helping in the contentions that we are chasing here, but I
thought of sharing the above idea to see if it is reliable and better than
revert option.

Thanks
 -- Imran