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: <ea0bc8e3-177c-f78a-de37-339e6261631c@oracle.com>
Date:   Wed, 6 Jul 2022 06:18:28 +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>
 <deaaabe5-0a0c-b6f9-c85b-5080874f6437@oracle.com>
 <YsSD4k0o6PPYpMBe@slm.duckdns.org>
From:   Imran Khan <imran.f.khan@oracle.com>
In-Reply-To: <YsSD4k0o6PPYpMBe@slm.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?cGJrNmRSVmI0Tnp4TVo1MWJ4cVpKNTYzMHRoQUZYanpyYmhQeFZweXBENnlq?=
 =?utf-8?B?WGJRRDV4VWJwNXQrSVF5VDd5bTM0endTaFhHdTc2RVpiWHZaL2JlNWtIcUdG?=
 =?utf-8?B?Z0IrY3Z1dWl1bysyNlJRQldJNHlwaW5rVjdUSjBMZ3psT3ZRZlFEWllnY1Ri?=
 =?utf-8?B?K05TamtEMitzazNPYmlUTDlEd0Nxd0JzQUxWZnpVOUlTSlB1ZTN5Zk4wL2VH?=
 =?utf-8?B?OE5WM3A2NGZMZUpKSFJiaFdHK24yRUhHWndRNnY3dllEVHY4dzFZTm9TMWsw?=
 =?utf-8?B?a1Ntd0l0TnF2d2Z2aGdxQWVqTzFMS0N4bHRWUzR4NFNWUzFTQmJrUCtGclU2?=
 =?utf-8?B?Y0NydEdybCtqUmF6dHhTUlZxY3h1eVVUMWNFMUhqN2FpSzFOTXV6YVpsa1lK?=
 =?utf-8?B?STRIRXNRZzNsSGEwQmxhY2tRNjUzbWVvbkNCRWtCR1NDTDVQNzZaaGVJRCt4?=
 =?utf-8?B?V082WENwTjNUUnVVa0k1UW1Cd3lZY1ZNRVlWeTJJYW9ZTVJhM2ZMQnpaaHlH?=
 =?utf-8?B?QmNVdUlOSzI1NzFMYjJZVVB6OTlUZ2tnR1NMNjE3RlBJOHZvWjlwVzk2Rm85?=
 =?utf-8?B?aTlJRnBSbjM3NUhzOG1Qa3psVjdOSjhXeEJLTnVXR2l2c3YwdVpyR080ZlBj?=
 =?utf-8?B?WjVkRDVNYUp0bjFsQUZENVg5UDZqZDFKUXRtTXRyTUdnR1JaK3UweUhReHZt?=
 =?utf-8?B?YlBqK3EwaUc4M3g5NDRzdFIwM29tOWY4Qm5wWDVFOFBZOW5PZFBKcXhBTW5Z?=
 =?utf-8?B?cHoyQU8rdFBRdUFNSEZ1ckdyL1hrbkJXVnFkbHE4ZXgzU0lqTXRUTU5Cb29h?=
 =?utf-8?B?Wm9DcXovOXo5d2IyaFBWRFU1UENPejI0K2phaG5sMDNIc01iUUdocExFVmxI?=
 =?utf-8?B?K3F4cWxKOWtjUGs1R3lkUVB4MVlNSy9sNDk3eW5VNVBFS3FPMW5udDh1VjZu?=
 =?utf-8?B?dmlISjQrbUhxcDRQVVdDR1RKeHVvbXIvL3FOWHdrY3ZhaWVEWGJ5OEhNVUFQ?=
 =?utf-8?B?STdGa1BUM1l5ZzJhekh5QVBiVk1hZjY0d0l0VVVua09rVmo2UUVXMTJmeWlU?=
 =?utf-8?B?NXd1NTJJaTZBTHl5cUYvSTZmUGVrSzd2ZEFpcDhCQjlMMUxrUFUzSG9yVmow?=
 =?utf-8?B?YlpOa2RLRlArMGRwaDUzYjNSS1B0M3FkSXdKZ3NnODJZVFFUckJwSzhLRlpL?=
 =?utf-8?B?dzlYUk5TakgySlQ1SElJSGI0Ni9VS3ppbXlmUG9nRmlzQ0RVemJKNVJMaXY5?=
 =?utf-8?B?eHVzOGhHSk9hZDFJVGdZbHhWNlFoTWpuK29PdzZyenkzbkdVUXJvbWk0dU9j?=
 =?utf-8?B?NW1hMkZ5b0RrRUVGVEVOcndkK3RJbXpTQjdueXJyNWF4MGU1S0RyZ203YitO?=
 =?utf-8?B?NXZIak1RbHFtcDNZU3lUdTRPYzVXUE1YcE1ZN2NLVXVWMnI2T1pnNldkWXNx?=
 =?utf-8?B?VmFUUVUxa0FiNk9UcjVSdklnc2ViV2FQMXlUenBvS2V0QS94Wm52ZS9PVVhH?=
 =?utf-8?B?SlJqZFFPMnppNkZzdEtCVHNWa3g0T09ZMWtzckZzcEdKdzMzZGxQa09qTEg0?=
 =?utf-8?B?L3dnOEZWWER0QzZaNS84cjZrWDhLNmdDNmYxRHREOEJJWmJIU1B6WVpIQXBu?=
 =?utf-8?B?TS9NWHNwQ3FqSityUmdBaHgwSTNXWWVUcjZMSGlZMTNhbkQ1Q0dSOEZwSmFH?=
 =?utf-8?B?NDNIQWdNdkUzYVJFM2VTdExDRHhQR1hrYVlwSGtGdmdJYXFWb1JSZ013cURH?=
 =?utf-8?B?UmRWa25rclRaenV1ZjdoWGNvend4VzBELzNCeEJ0U0psYzE2aGN0MWQxdVd0?=
 =?utf-8?B?QkFRdUhuMm9PbklEelJpK0JQY3VXcTlwY0J5K3EwRW5Fc2dTcDlCclQ4c1U3?=
 =?utf-8?B?OEF0WUpuS25EZzNzTzRsRG5GTko1c3ZaZ21GblZ5UGpjcEp3M0tvRVVLU0VG?=
 =?utf-8?B?SG5Dd2x6NVVJVDQ4Uk9MdHFlVDRLRExjMVhFeDN2VGJLbGVZN1BheXI3YkRi?=
 =?utf-8?B?KzhpN0hyYUk4ZHpTWXBVM0FwMktTTGkxZEVHaHM4OURBODc3Ny9VdkdLQUJ2?=
 =?utf-8?B?WWM5S2cxclVRWGJ6cmV3L1NOa2UwZE0yd0hRUDJKRkRXRVBwNjdiKzR5Lzdt?=
 =?utf-8?B?U3NYWjFudnV3N2VUSjBPNjZ5Tm9FWFhSazZOWWN1YmIxcENOeStvRXl5Y0Rw?=
 =?utf-8?B?T2c9PQ==?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b8c8b256-b38d-48b9-4294-08da5ec38aad
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4468.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jul 2022 20:18:36.7558
 (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: LgJ3CbYkkbSLeAqNRcT+PR6SMAnMx0NVmhv7r/4V1xZ0sEzwkie1hxIYmWjsjQsN5BuNpEtL4DUjDc2BC3pYLw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4694
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-07-05_18:2022-06-28,2022-07-05 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 suspectscore=0 spamscore=0
 malwarescore=0 mlxlogscore=999 phishscore=0 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207050088
X-Proofpoint-GUID: PXotXwAixqAZvJHny6kXx7G3SE96Vk1S
X-Proofpoint-ORIG-GUID: PXotXwAixqAZvJHny6kXx7G3SE96Vk1S
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,

On 6/7/22 4:33 am, Tejun Heo wrote:
> Hello,
> 
> On Sun, Jul 03, 2022 at 09:09:05PM +1000, Imran Khan wrote:
>> 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);
> 
> But then what's the point of using llist?
> 

In this case, the point of using llist would be to avoid taking the locks in
consumer.
>> 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.
> 
> It supports multiple producers in the sense that multiple producers can try
> to add their own llist_nodes concurrently. It doesn't support multiple
> producers trying to add the same llist_node whether that depends on NULL
> check or not.
> 

Hmm. My idea was that eventually we will never run into situation where multiple
producers will end up adding the same node because as soon as first producer
adds the node (the other potential adders are spinning on kernfs_notify_lock),
kn->attr.notif_next.next will get a non-NULL value and checking
(kn->attr.notify_next.next != NULL) will avoid the node getting re-added.

I must be missing something here, so as per your suggestion I have reverted this
change at [1].

Thanks,
 -- Imran

[1]: https://lore.kernel.org/lkml/20220705201026.2487665-1-imran.f.khan@oracle.com/