Re: [PATCH 1/2] brw_mutex: big read-write mutex

[Oleg Nesterov]

Hi Mikulas,

On 10/22, Mikulas Patocka wrote:
>
> On Fri, 19 Oct 2012, Oleg Nesterov wrote:
>
> > On 10/19, Mikulas Patocka wrote:
> > >
> > > synchronize_rcu() is way slower than msleep(1) -
> >
> > This depends, I guess. but this doesn't mmatter,
> >
> > > so I don't see a reason
> > > why should it be complicated to avoid msleep(1).
> >
> > I don't think this really needs complications. Please look at this
> > patch for example. Or initial (single writer) version below. It is
> > not finished and lacks the barriers too, but I do not think it is
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

please note the comment above ;)

> > more complex.
>
> Hi
>
> My implementation has a smaller structure (it doesn't have
> wait_queue_head_t).

Oh, I don't think sizeof() really matters in this case.

> Your implementation is prone to starvation - if the writer has a high
> priority and if it is doing back-to-back write unlocks/locks, it may
> happen that the readers have no chance to run.

Yes, it is write-biased, this was intent. writers should be rare.

> The use of mutex instead of a wait queue in my implementation is unusual,
> but I don't see anything wrong with it

Neither me.

Mikulas, apart from _rcu/_sched change, my only point was msleep() can
(and imho should) be avoided.

> > static inline long brw_read_ctr(struct brw_sem *brw)
> > {
> > 	long sum = 0;
> > 	int cpu;
> >
> > 	for_each_possible_cpu(cpu)
> > 		sum += per_cpu(*brw->read_ctr, cpu);
>
> Integer overflow on signed types is undefined - you should use unsigned
> long - you can use -fwrapv option to gcc to make signed overflow defined,
> but Linux doesn't use it.

I don't think -fwrapv can make any difference in this case, but I agree
that "unsigned long" makes more sense.

> > void brw_up_write(struct brw_sem *brw)
> > {
> > 	brw->writer = NULL;
> > 	synchronize_sched();
>
> That synchronize_sched should be put before brw->writer = NULL.

Yes, I know. I mentioned this at the start, this lacks the necessary
barrier between this writer and the next reader.

> I had this bug in my implementation too.

Yes, exactly. And this is why I cc'ed you initially ;)

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/