blk-softirq vs smp_call_function_single_async()

blk-softirq vs smp_call_function_single_async()

[Peter Zijlstra]

Hi Jens,

I've been going through smp_call_function_single_async() users and
stumbled upon blk-softirq.c, which has:

static int raise_blk_irq(int cpu, struct request *rq)
{
	if (cpu_online(cpu)) {
		call_single_data_t *data = &rq->csd;

		data->func = trigger_softirq;
		data->info = rq;
		data->flags = 0;

		smp_call_function_single_async(cpu, data);
		return 0;
	}

	return 1;
}

What, if anything, guarantees rq->csd is not already in use at that
time?

The purpose of that CSD is to make the BLOCK_SOFTIRQ go, but there's
plenty of other ways to tickle that, afaict. So if that races vs someone
else, and that completes whatever was needed, then can't we get to
raise_blk_irq() again, even though the csd is still enqueued?

Worse; it has: data->flags = 0; so our early exit will not happen, even
when it should.

Re: blk-softirq vs smp_call_function_single_async()

[Christoph Hellwig]

On Mon, Jun 08, 2020 at 01:58:00PM +0200, Peter Zijlstra wrote:
> Hi Jens,
> 
> I've been going through smp_call_function_single_async() users and
> stumbled upon blk-softirq.c, which has:
> 
> static int raise_blk_irq(int cpu, struct request *rq)
> {
> 	if (cpu_online(cpu)) {
> 		call_single_data_t *data = &rq->csd;
> 
> 		data->func = trigger_softirq;
> 		data->info = rq;
> 		data->flags = 0;
> 
> 		smp_call_function_single_async(cpu, data);
> 		return 0;
> 	}
> 
> 	return 1;
> }
> 
> What, if anything, guarantees rq->csd is not already in use at that
> time?

A request can only be completed once.

Re: blk-softirq vs smp_call_function_single_async()

[Peter Zijlstra]

On Mon, Jun 08, 2020 at 08:45:57AM -0700, Christoph Hellwig wrote:
> On Mon, Jun 08, 2020 at 01:58:00PM +0200, Peter Zijlstra wrote:
> > Hi Jens,
> > 
> > I've been going through smp_call_function_single_async() users and
> > stumbled upon blk-softirq.c, which has:
> > 
> > static int raise_blk_irq(int cpu, struct request *rq)
> > {
> > 	if (cpu_online(cpu)) {
> > 		call_single_data_t *data = &rq->csd;
> > 
> > 		data->func = trigger_softirq;
> > 		data->info = rq;
> > 		data->flags = 0;
> > 
> > 		smp_call_function_single_async(cpu, data);
> > 		return 0;
> > 	}
> > 
> > 	return 1;
> > }
> > 
> > What, if anything, guarantees rq->csd is not already in use at that
> > time?
> 
> A request can only be completed once.

Sure, but that doesn't help.

CPU0				CPU1

 raise_blk_irq()		BLOCK_SOFTIRQ
   IPI -> CPU1

				// picks up thing from CPU0
				req->complete(req);


	<big hole where CSD is active and request completed>

				<IPI>
				  trigger_softirq()


What happens to a struct request after completion, is it free()d
or reused? If reused, how do we guarantee CSD completion before
free()ing?

Re: blk-softirq vs smp_call_function_single_async()

[Christoph Hellwig]

On Mon, Jun 08, 2020 at 05:58:33PM +0200, Peter Zijlstra wrote:
> > A request can only be completed once.
> 
> Sure, but that doesn't help.
> 
> CPU0				CPU1
> 
>  raise_blk_irq()		BLOCK_SOFTIRQ
>    IPI -> CPU1
> 
> 				// picks up thing from CPU0
> 				req->complete(req);
> 
> 
> 	<big hole where CSD is active and request completed>
> 
> 				<IPI>
> 				  trigger_softirq()
> 
> 
> What happens to a struct request after completion, is it free()d
> or reused? If reused, how do we guarantee CSD completion before
> free()ing?

The request is freed in the block layer sense by __blk_mq_free_request
(which doesn't actually free the memory, so it eventually gets reused).

__blk_mq_free_request is called from blk_mq_end_request which
is called from the drivers ->complete_rq method, which is called
from the block softirq.

What is the method to guaranteed CSD completion?

Re: blk-softirq vs smp_call_function_single_async()

[Peter Zijlstra]

On Mon, Jun 08, 2020 at 09:33:42AM -0700, Christoph Hellwig wrote:
> On Mon, Jun 08, 2020 at 05:58:33PM +0200, Peter Zijlstra wrote:
> > > A request can only be completed once.
> > 
> > Sure, but that doesn't help.
> > 
> > CPU0				CPU1
> > 
> >  raise_blk_irq()		BLOCK_SOFTIRQ
> >    IPI -> CPU1
> > 
> > 				// picks up thing from CPU0
> > 				req->complete(req);
> > 
> > 
> > 	<big hole where CSD is active and request completed>
> > 
> > 				<IPI>
> > 				  trigger_softirq()
> > 
> > 
> > What happens to a struct request after completion, is it free()d
> > or reused? If reused, how do we guarantee CSD completion before
> > free()ing?
> 
> The request is freed in the block layer sense by __blk_mq_free_request
> (which doesn't actually free the memory, so it eventually gets reused).
> 
> __blk_mq_free_request is called from blk_mq_end_request which
> is called from the drivers ->complete_rq method, which is called
> from the block softirq.
> 
> What is the method to guaranteed CSD completion?

There isn't one, it was meant to be used with static allocations.

Frederic proposed changing all these to irq_work, and I think I'll go do
that. First dinner though.

Re: blk-softirq vs smp_call_function_single_async()

[Christoph Hellwig]

On Mon, Jun 08, 2020 at 06:40:09PM +0200, Peter Zijlstra wrote:
> There isn't one, it was meant to be used with static allocations.
> 
> Frederic proposed changing all these to irq_work, and I think I'll go do
> that. First dinner though.

The irq_work API looks reasonable.  What are the tradeoffs for
smp_call_single_async vs irq_work?

Re: blk-softirq vs smp_call_function_single_async()

[Jens Axboe]

On 6/8/20 10:40 AM, Peter Zijlstra wrote:
> On Mon, Jun 08, 2020 at 09:33:42AM -0700, Christoph Hellwig wrote:
>> On Mon, Jun 08, 2020 at 05:58:33PM +0200, Peter Zijlstra wrote:
>>>> A request can only be completed once.
>>>
>>> Sure, but that doesn't help.
>>>
>>> CPU0				CPU1
>>>
>>>  raise_blk_irq()		BLOCK_SOFTIRQ
>>>    IPI -> CPU1
>>>
>>> 				// picks up thing from CPU0
>>> 				req->complete(req);
>>>
>>>
>>> 	<big hole where CSD is active and request completed>
>>>
>>> 				<IPI>
>>> 				  trigger_softirq()
>>>
>>>
>>> What happens to a struct request after completion, is it free()d
>>> or reused? If reused, how do we guarantee CSD completion before
>>> free()ing?
>>
>> The request is freed in the block layer sense by __blk_mq_free_request
>> (which doesn't actually free the memory, so it eventually gets reused).
>>
>> __blk_mq_free_request is called from blk_mq_end_request which
>> is called from the drivers ->complete_rq method, which is called
>> from the block softirq.
>>
>> What is the method to guaranteed CSD completion?
> 
> There isn't one, it was meant to be used with static allocations.
> 
> Frederic proposed changing all these to irq_work, and I think I'll go do
> that. First dinner though.

That sounds good to me, and will also shrink struct request a bit,
which is always nice.

-- 
Jens Axboe

Re: blk-softirq vs smp_call_function_single_async()

[Peter Zijlstra]

On Mon, Jun 08, 2020 at 09:42:54AM -0700, Christoph Hellwig wrote:
> On Mon, Jun 08, 2020 at 06:40:09PM +0200, Peter Zijlstra wrote:
> > There isn't one, it was meant to be used with static allocations.
> > 
> > Frederic proposed changing all these to irq_work, and I think I'll go do
> > that. First dinner though.

OK, after having looked at this more, I think my initial analysis is
actually wrong and this code should work as-is.

The thing that I missed yesterday is that we only add the request to the
blk_cpu_done list in the IPI, this means that the race I described
earlier is not in fact possible.

The IPI must happen for progress to be made.

And the same is true for blk_mq_force_complete_rq(), which also uses
this csd.

> The irq_work API looks reasonable.  What are the tradeoffs for
> smp_call_single_async vs irq_work?

To still answer your question; irq_work_queue*() has an atomic op extra
that allows for more convenient semantics -- but is in your case
strictly superfluous.

Still, Jens' point about irq_work being smaller stands, and I think more
users could benefit from something intermediate. Let me continue with
the cleanups / audit and see what comes out at the end.

Thanks!