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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121023151224.GA14916@redhat.com>
Date:	Tue, 23 Oct 2012 17:12:24 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Mikulas Patocka <mpatocka@...hat.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anton Arapov <anton@...hat.com>, linux-kernel@...r.kernel.org,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 1/2] brw_mutex: big read-write mutex

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/