lkml.org 
[lkml]   [2002]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH]updated ipc lock patch
Rusty Russell wrote:
>
>
> Here's my brief audit:
>
> >+ int max_id = ids->max_id;
> >
> >- for (id = 0; id <= ids->max_id; id++) {
> >+ read_barrier_depends();
> >+ for (id = 0; id <= max_id; id++) {
>
> That needs to be a rmb(), not a read_barrier_depends().

Thanks for spending some time reviewing the barriers for me. While I was
thinking the reason why a rmb is needed here, I found that maybe we
don't need a barrier here at all. Since ipc_findkey()(the code above)
and the grow_ary() are both protected by ipc_ids.sem(there missing
document for this), so both the max_id and the the entries array seen by
ipc_findkey should be the latest one.

Also I think it's safe to remove the rmb() in ipc_get() for the same
reason. ipc_get() is only used by shm_get_stat() through shm_get() and
is called with the shm_ids.sem protected. (Maybe ipc_get should be
removed totally?)

> And like all
> barriers, it *requires* a comment:
> /* We must read max_id before reading any entries */
>
Sure. I will add such comments on all places where barriers are being
used. I will do as much as I can to add more comments in the code about
what lock/sem are hold before/after the funtion is called.:-)

> I can't see the following in the patch posted, but:
> > void ipc_rcu_free(void* ptr, int size)
> > {
> > struct rcu_ipc_free* arg;
> >
> > arg = (struct rcu_ipc_free *) kmalloc(sizeof(*arg), GFP_KERNEL);
> > if (arg == NULL)
> > return;
> > arg->ptr = ptr;
> > arg->size = size;
> > call_rcu(&arg->rcu_head, ipc_free_callback, arg);
> > }
>
> This is unacceptable crap, sorry. You *must* allocate the resources
> required to free the object *at the time you allocate the object*,
> since freeing must not fail.
>
> > Even better: is it possible to embed the rcu_ipc_free inside the
> > object-to-be-freed? Perhaps not?
>
> Yes, this must be done.
>
I thought about embed rcu_ipc_free inside the ipc_ids structure before.
But there could be a problem if grow_ary() is called again before the
old array associated with the previous grow_ary() has not scheduled to
be freed yet. I see a need to do that now, as you made very good point.
I will make the changes tomorrow.

Thanks a lot for your comments.

Mingming
-
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/

\
 
 \ /
  Last update: 2005-03-22 13:30    [W:0.076 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site