From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754047Ab0KJUmE (ORCPT ); Wed, 10 Nov 2010 15:42:04 -0500 Received: from sj-iport-4.cisco.com ([171.68.10.86]:31648 "EHLO sj-iport-4.cisco.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753741Ab0KJUmC (ORCPT ); Wed, 10 Nov 2010 15:42:02 -0500 Authentication-Results: sj-iport-4.cisco.com; dkim=neutral (message not signed) header.i=none X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AvsEAGaS2kyrR7Hu/2dsb2JhbACiNnGjRZtGhUoEhFqFfQ X-IronPort-AV: E=Sophos;i="4.59,180,1288569600"; d="scan'208";a="215190743" Message-ID: <4CDB0399.9000206@cisco.com> Date: Wed, 10 Nov 2010 12:42:01 -0800 From: Joe Eykholt User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.12) Gecko/20101027 Lightning/1.0b2 Thunderbird/3.1.6 MIME-Version: 1.0 To: Vladislav Bolkhovitin CC: Boaz Harrosh , Greg KH , linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, scst-devel , James Bottomley , Andrew Morton , FUJITA Tomonori , Mike Christie , Vu Pham , Bart Van Assche , James Smart , Andy Yan , Chetan Loke , Dmitry Torokhov , Hannes Reinecke , Richard Sharpe , Daniel Henrique Debonzi Subject: Re: [PATCH 8/19]: SCST SYSFS interface implementation References: <20101011213235.GA11489@kroah.com> <4CB4AEB9.30501@vlnb.net> <20101012190345.GA25737@kroah.com> <4CB75E81.7000208@vlnb.net> <20101014200413.GA30831@kroah.com> <4CC1CA4D.1090609@vlnb.net> <20101022175624.GA13640@kroah.com> <4CC1DAA2.7030602@vlnb.net> <20101022185437.GA9103@kroah.com> <4CD8566D.1020202@vlnb.net> <20101109002829.GA22633@kroah.com> <4CD9A9B8.70708@vlnb.net> <4CDA6CD4.3010308@panasas.com> <4CDAFE6E.7050200@vlnb.net> <4CDB00C2.2090200@cisco.com> In-Reply-To: <4CDB00C2.2090200@cisco.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/10/10 12:29 PM, Joe Eykholt wrote: > > > On 11/10/10 12:19 PM, Vladislav Bolkhovitin wrote: >> Boaz Harrosh, on 11/10/2010 12:58 PM wrote: >>> On 11/09/2010 10:06 PM, Vladislav Bolkhovitin wrote: >>>> >>>> Sorry, but what is incorrect in the working implementation without any >>>> bugs doing its job in the simplest, smallest and clearest way? >>>> >>>> If those objects remade to free themselves in the kobjects release(), >>>> what value would it add to them? Would the implementation be simpler, >>>> smaller or clearer? Not, I believe, new implementation would be only >>>> bigger and less clear. So, what's the point to do it to make the code worse? >>>> >>> >>> Totally theoretically speaking, since I have not inspected the code. >>> >>> If today you wait for the count to reach zero, then unregister >>> and send an event to some other subsystem to free the object. >>> >>> Is it not the same as if you take an extra refcount, unregister and >>> send the event at count=1. Then at that other place decrement the last >>> count to cause the object to be freed. >>> >>> I agree that it is hard to do lockless. what some places do is have >>> an extra kref. The kobj has a single ref on it. everything takes the >>> other kref. when that reaches zero the unregister and event fires >>> and at free you decrement the only kobj ref to deallocate. This is one >>> way. In some situations you can manage with a single counter it all >>> depends. >> >> Thanks for sharing your thoughts with us. But the question isn't about >> if it's possible to implement what we need locklessly. The question is >> in two approaches how to synchronously delete objects with entries on SYSFS: >> >> 1. struct object_x { >> ... >> struct kobject kobj; >> struct completion *release_completion; >> }; >> >> static void x_release(struct kobject *kobj) >> { >> struct object_x *x; >> struct completion *c; >> >> x = container_of(kobj, struct object_x, kobj); >> c = x->release_completion; >> kfree(x); >> complete_all(c); >> } >> >> void del_object(struct object_x *x) >> { >> DECLARE_COMPLETION_ONSTACK(completion); >> >> ... >> x->release_completion = &completion; >> kobject_put(&x->kobj); >> wait_for_completion(&completion); >> } >> >> and >> >> 2. struct object_x { >> ... >> struct kobject kobj; >> struct completion release_completion; >> }; >> >> static void x_release(struct kobject *kobj) >> { >> struct object_x *x; >> >> x = container_of(kobj, struct object_x, kobj); >> complete_all(&x->release_completion); >> } >> >> void del_object(struct object_x *x) >> { >> ... >> kobject_put(&x->kobj); >> wait_for_completion(&completion); >> ... >> kfree(x); >> } > > I'll admit I don't understand this all that well, but > why not just have x_release() (based on (2)) > do free(x), and have del_object > do the kobject_put(&x->kobj) as its very last thing? > Then you don't need the completion. Ah, well to answer my own question, I guess that's (1). Never mind. Joe >> Greg asserts that (1) is the only correct approach while (2) is >> incorrect, and I'm trying to justify that (2) is correct too and >> sometimes could be better, i.e. simpler and clearer, because it >> decouples object_x from SYSFS and its kobj. Then kobj becomes an >> ordinary member of struct object_x without any special treatment and >> with the same lifetime rules as other members of struct object_x. While >> in (1) all lifetime of struct object_x is strictly attached to kobj, so >> it needs be specially handled with additional code for that if struct >> object_x has many other members which needed to be initialized/deleted >> _before and after_ kobj as we have in SCST. >> >> Vlad