Message-ID: <4CDAFE6E.7050200@vlnb.net>
Date: Wed, 10 Nov 2010 23:19:58 +0300
From: Vladislav Bolkhovitin <vst@vlnb.net>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.10) Gecko/20100527 Thunderbird/3.0.5
MIME-Version: 1.0
To: Boaz Harrosh <bharrosh@panasas.com>
CC: Greg KH <greg@kroah.com>, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        scst-devel <scst-devel@lists.sourceforge.net>,
        James Bottomley <James.Bottomley@HansenPartnership.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>,
        Mike Christie <michaelc@cs.wisc.edu>, Vu Pham <vuhuong@mellanox.com>,
        Bart Van Assche <bart.vanassche@gmail.com>,
        James Smart <James.Smart@Emulex.Com>, Joe Eykholt <jeykholt@cisco.com>,
        Andy Yan <ayan@marvell.com>, Chetan Loke <generationgnu@yahoo.com>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Hannes Reinecke <hare@suse.de>,
        Richard Sharpe <realrichardsharpe@gmail.com>,
        Daniel Henrique Debonzi <debonzi@linux.vnet.ibm.com>
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>
In-Reply-To: <4CDA6CD4.3010308@panasas.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3284
Lines: 101

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);
}

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/