Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 20 Jan 2023 09:25:00 +0100
From:   Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To:     Mel Gorman <mgorman@techsingularity.net>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Linux-RT <linux-rt-users@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] locking/rwbase: Prevent indefinite writer starvation
Message-ID: <Y8pP3CD1PQ4KWhXF@linutronix.de>
References: <20230117083817.togfwc5cy4g67e5r@techsingularity.net>
 <Y8avJm1FQI9vB9cv@linutronix.de>
 <20230117165021.t5m7c2d6frbbfzig@techsingularity.net>
 <Y8gPhTGkfCbGwoUu@linutronix.de>
 <20230118173130.4n2b3cs4pxiqnqd3@techsingularity.net>
 <Y8j+lENBWNWgt4mf@linutronix.de>
 <20230119110220.kphftcehehhi5l5u@techsingularity.net>
 <Y8lvwKHmmnikVDgk@linutronix.de>
 <20230119174101.rddtxk5xlamlnquh@techsingularity.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230119174101.rddtxk5xlamlnquh@techsingularity.net>
Precedence: bulk

On 2023-01-19 17:41:01 [+0000], Mel Gorman wrote:
> 
> Yes, it makes your concern much clearer but I'm not sure it actually matters
> in terms of preventing write starvation or in terms of correctness. At
> worst, a writer is blocked that could have acquired the lock during a tiny
> race but that's a timing issue rather than a correctness issue.

Correct. My concern is that one reader may need to wait 4ms+ for the
lock while a following reader (that one that sees the timeout) does not.
This can lead to confusion later on.

> Lets say the race hits
> 
> 									reader sees waiter_timeout == 0
> 	writer acquires wait_lock
> 	__rwbase_write_trylock fails
> 	update waiter_timeout
> 	rwbase_schedule
> 
> Each reader that hits the race goes ahead at a point in time but anything
> readers after that observe the timeout and eventually the writer goes ahead.
> 
> If the waiter_timeout was updated before atomic_sub(READER_BIAS),
> it doesn't close the race as atomic_sub is unordered so barriers would
> also be needed and clearing of waiter_timeout moves to out_unlock in case
> __rwbase_write_trylock succeeds. That's possible but the need for barriers
> makes it more complicated than is necessary.

yes...

> The race could be closed by moving wait_lock acquisition before the
> atomic_sub in rwbase_write_lock() but it expands the scope of the wait_lock
> and I'm not sure that's necessary for either correctness or preventing
> writer starvation. It's a more straight-forward fix but expanding the
> scope of a lock unnecessarily has been unpopular in the past.
> 
> I think we can close the race that concerns you but I'm not convinced we
> need to and changing the scope of wait_lock would need a big comment and
> probably deserves a separate patch.

would it work to check the timeout vs 0 before and only apply the
timeout check if it is != zero? The writer would need to unconditionally
or the lowest bit. That should close gaps at a low price. The timeout
variable is always read within the lock so there shouldn't be need for
any additional barriers.

> Sorry if I'm still missing something stupid and thanks for your patience
> reviewing this.
thank that it is patience and not pain in the ass ;)

Sebastian