On Fri, 2007-03-30 at 00:05 -0700, Greg KH wrote:
> On Fri, Mar 30, 2007 at 02:33:44PM +0800, Shaohua Li wrote:
> > On Thu, 2007-03-29 at 22:18 -0700, Greg KH wrote:
> > > On Wed, Mar 28, 2007 at 02:49:12PM +0800, Shaohua Li wrote:
> > > > On Tue, 2007-03-27 at 22:58 -0700, Greg KH wrote:
> > > > > On Wed, Mar 28, 2007 at 01:39:26PM +0800, Shaohua Li wrote:
> > > > > > On Tue, 2007-03-27 at 22:27 -0700, Greg KH wrote:
> > > > > > >
> > > > > > > Putting more than one kobject in the same structure is a broken design.
> > > > > > > How can you control the lifetime rules properly if there are two
> > > > > > > reference counts for the same structure? It doesn't work.
> > > > > > >
> > > > > > > If you really need something like this, then just use a pointer to a
> > > > > > > kobject for one of them instead of embedding it. Why do you need two
> > > > > > > different kobjects here?
> > > > > > Our data structure is something like below:
> > > > > >
> > > > > > struct foo {
> > > > > > kobject kobja;
> > > > > > }
> > > > > >
> > > > > > struct bar {
> > > > > > struct foo foo[];
> > > > >
> > > > > Ick, don't do that...
> > > > why?
> > > > > > kobject kobjb
> > >
> > > Because you have multiple kobjects in the same object.
> > >
> > > It's just that simple, the lifetime rules for such a thing is almost
> > > impossible to track properly. Don't do it!
> > >
> > > > > > }
> > > > > >
> > > > > > kobjb's .release will free struct bar. kobjb is the parent of kobja. if
> > > > > > you have a reference on kobja, then kobjb can't be released too, right?
> > > > > > So we only kobjb provide a .release to free the memory, kobja's .release
> > > > > > isn't required.
> > > > >
> > > > > Why not just use the "normal" parent/child relationship with the
> > > > > kobjects like the rest of the kernel does?
> > > > I still didn't get the reason why we couldn't do this in the way of my
> > > > patch. As I said, there isn't risk to use 'freed memory'. I can make the
> > > > 'struct foo' a pointer, but this will mess the cpuidle driver.
> > >
> > > Again, the main point is you can not have more than one reference count
> > > for the same structure. It just does not work at all.
> > >
> > > So please, fix the code, it is broken.
> > >
> > > And yes, I know of other places in the kernel (scsi stack...) that
> > > violate this, but that only means that they are wrong, not that it is an
> > > excuse for you to do it also.
> > We don't use the kobject to track the reference count.
>
> Then what do you use it for?
>
> And it doesn't matter if you use it or not, the reference count _is_
> used by the kobject and sysfs code, so you have to be aware of it.
>
> > But anyway, below patch should make you happy.
>
> I'm still confused as to why you are creating these "extra" kobjects.
> What are they used for?
It's for each idle state.
> But yes, it is "better" in that you are abiding by the reference count
> rules properly, although your completion handling seems like it might
> get into a deadlock, but I'll trust you that it works properly :)
Yes, it works well in my test.
Thanks,
Shaohua
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
