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;
Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
        Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
        Kingdom.
        Registered in England and Wales under Company Registration No. 3798903
From:   David Howells <dhowells@redhat.com>
In-Reply-To: <20221218120951.1212-1-hdanton@sina.com>
References: <20221218120951.1212-1-hdanton@sina.com> <20221216001958.1149-1-hdanton@sina.com> <167112117887.152641.6194213035340041732.stgit@warthog.procyon.org.uk>
To:     Hillf Danton <hdanton@sina.com>
Cc:     dhowells@redhat.com, netdev@vger.kernel.org,
        Marc Dionne <marc.dionne@auristor.com>,
        linux-afs@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net 7/9] rxrpc: Fix I/O thread stop
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1017323.1671409205.1@warthog.procyon.org.uk>
Date:   Mon, 19 Dec 2022 00:20:05 +0000
Message-ID: <1017324.1671409205@warthog.procyon.org.uk>
Precedence: bulk

Hillf Danton <hdanton@sina.com> wrote:

> > So once we've observed that we've been asked to stop, we need to check if
> > there's more work to be done and, if so, do that work first.
> 
> In line with
> 
> 	if (condition)
> 		return;
> 	add to wait queue
> 	if (!condition)
> 		schedule();
> 
> this change should look like
> 
>    		if (!skb_queue_empty(&local->rx_queue) ...)
>  			continue;
> 
>  		if (kthread_should_stop())
>    			if (!skb_queue_empty(&local->rx_queue) ...)
>  				continue;
> 			else
>   				break;
> 
> as checking condition once barely makes sense.

Note that these are not really analogous.  The add-to-wait-queue step is
significantly more expensive than kthread_should_stop() and requires removal
in the event that the condition becomes true in the window.

In the case of kthread_should_stop(), it's just a test_bit() of a word that's
in a cacheline not going to get changed until the thread is stopped.  Testing
the value first and then checking the condition should be fine as the stop
flag can be shared in the cpu's data cache until it's set.

Also from a code-maintenance PoV, I don't want to write the condition twice if
I can avoid it.  That allows for the two copies to get out of sync.

> Because of a bit complex condition does not mean checking it once is neither
> sane nor correct.

So you agree with me, I think?

David