Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756801Ab0KJU34 (ORCPT ); Wed, 10 Nov 2010 15:29:56 -0500 Received: from sj-iport-2.cisco.com ([171.71.176.71]:40269 "EHLO sj-iport-2.cisco.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756327Ab0KJU3z (ORCPT ); Wed, 10 Nov 2010 15:29:55 -0500 Authentication-Results: sj-iport-2.cisco.com; dkim=neutral (message not signed) header.i=none X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AvsEAFuP2kyrR7H+/2dsb2JhbACiNnGjN5tGhUoEhFqFfQ X-IronPort-AV: E=Sophos;i="4.59,179,1288569600"; d="scan'208";a="290357690" Message-ID: <4CDB00C2.2090200@cisco.com> Date: Wed, 10 Nov 2010 12:29:54 -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> In-Reply-To: <4CDAFE6E.7050200@vlnb.net> 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 Content-Length: 3721 Lines: 111 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/