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;
Feedback-ID: iad51458e:Fastmail
Date:   Mon, 15 Aug 2022 12:24:39 -0700
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Hector Martin <marcan@marcan.st>
Cc:     Will Deacon <will@kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Greg KH <gregkh@linuxfoundation.org>, jirislaby@kernel.org,
        Marc Zyngier <maz@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Asahi Linux <asahi@lists.linux.dev>,
        Oliver Neukum <oneukum@suse.com>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: Debugging a TTY race condition on M1 (memory ordering dragons)
Message-ID: <Yvqddx6nTRSZ0JYP@boqun-archlinux>
References: <6c089268-4f2c-9fdf-7bcb-107b611fbc21@marcan.st>
 <20220815134711.GA10374@willie-the-truck>
 <63cd54a8-3c48-d1b9-406a-c521bd02ee4a@marcan.st>
 <YvqKtJn5eBsDJXBI@boqun-archlinux>
 <8ccfdd2d-ef77-4586-e50c-985e1d13726a@marcan.st>
 <YvqXbApYbGGEqQl+@boqun-archlinux>
 <f3f84fc1-50be-0a9b-0538-6ea26dd93b16@marcan.st>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f3f84fc1-50be-0a9b-0538-6ea26dd93b16@marcan.st>
Precedence: bulk

On Tue, Aug 16, 2022 at 04:15:00AM +0900, Hector Martin wrote:
> On 16/08/2022 03.58, Boqun Feng wrote:
> > I agree this is handy, but an unconditional full barrier may be costy to
> > some users, and probably unnecessary if the users periodically queue
> > the work. In that case, some successful enqueue will eventually make all
> > memory accesses observable. Also if workqueue users use their own
> > locking in work function, then the barrier is also unnecessary.
> > 
> > The document part of course needs some help to clear things up. But I'm
> > not sure "strengthen"ing the ordering guarantee of queue_work() is a
> > good idea. Maybe a dedicated API, like:
> > 
> > // More work is needed for the @work, it has the same semantics as
> > // queue_work() if the @work is not pending. If the @work is pending,
> > // this ensures the work function observes all memory access before
> > // this.
> > void queue_more_work(struct work_struct *work)
> > {
> > 	smp_mb();
> > 	queue_work(work);
> > }
> > 
> > Regards,
> > Boqun
> 
> FWIW, I didn't actually use a full barrier in my patch. I just replaced
> the test_and_set_bit() with the underlying atomic op, sans early exit path.
> 
> Personally though, I think it makes more sense to have the default
> function provide the guarantees, and if someone *really* needs the
> performance gain from eliding the implicit barrier, they could use an
> alternate API for that (after they show useful gains). This stuff is too
> subtle to expect every caller to wrap their head around memory ordering,
> and having queue_work() always provide order with prior stores *feels*
> intuitive.
> 

Fair enough. It's just that when you know something about memory
ordering and atomics, you kinda want to play the game to save as many
barrier as you can ;-) It's a curse of knowledge.

> But let's see what the workqueue folks say :)
> 

Looks like they agree with you ;-) Nice work!

Regards,
Boqun

> - Hector