Re: [PATCH net 7/9] rxrpc: Fix I/O thread stop

[David Howells]

Hillf Danton <hdanton@xxxxxxxx> 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