Re: [PATCH 1/1] rpmsg: virtio_rpmsg_bus - prevent possible race condition

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

On 9/5/23 03:33, Tim Blechmann wrote:
>>>> when we cannot get a tx buffer (`get_a_tx_buf`) `rpmsg_upref_sleepers`
>>>> enables tx-complete interrupt.
>>>> however if the interrupt is executed after `get_a_tx_buf` and before
>>>> `rpmsg_upref_sleepers` we may mis the tx-complete interrupt and sleep
>>>> for the full 15 seconds.
>>>
>>>
>>> Is there any reason why your co-processor is unable to release the TX RPMSG
>>> buffers for 15 seconds? If not, you should first determine the reason why it is
>>> stalled.
>>
>> Arnaud's concern is valid.  If the remote processor can't consume a buffer
>> within 15 seconds, something is probably wrong.
>>
>> That said, I believe your assesment of the situation is correct.  *If* the TX
>> callback is disabled and there is no buffer available, there is a window of
>> opportunity between calls to get_a_tx_buf() and rpmsg_upref_sleepers() for an
>> interrupt to arrive in function rpmsg_send_offchannel_raw().
> 
> the remote processor certainly releases the tx buffer and according to my
> tracing the `vring_interrupt` fires immediately before `rpmsg_send` enters the
> `rpmsg_upref_sleepers`.


If I well understood your point, the issue occur in following race condition

- all the Tx buffers are used
- in rpmsg_send_offchannel_raw() function, we try to get a buffer using
get_a_tx_buf(vrp) that returns NULL
- rpmsg_xmit_done is called as a Tx buffer is released by the remote processor
  and now free
- in rpmsg_send_offchannel_raw() rpmsg_upref_sleepers is called

At this point you are nothing happen until 15 second because rpmsg_xmit_done is
never called  again that would wake up the waitqueue to call get_a_tx_buf()

I'm right?

If yes what is not clear to me is that wait_event_interruptible_timeout() seems
to test the condition (so call get_a_tx_buf()) before entering in sleep[1]. A
free TX buffer should be found at this step.

[1]https://elixir.bootlin.com/linux/latest/source/include/linux/wait.h#L534

Regards,
Arnaud

> 
> after applying this patch we haven't been able to reproduce the 15s timeout
> anymore, whereas before we could easily reproduce it with certain workloads.
> 
>> 3) This patch gets applied when rc1 comes out so that it has 6 or 7 weeks to
>> soak.  No error are locks are reported due to this patch during that time.
> 
> mentioning locks: i was a bit uncertain about a good way to implement the retry,
> since both `rpmsg_upref_sleepers` and `get_a_tx_buf` both acquire the same
> mutex. i briefly considered to add `get_a_tx_buf` into `rpmsg_upref_sleepers` to
> avoid locking the same mutex multiple times, though it adds a bit of complexity
> to the implementation and harms readability a bit.
> are there any recommendations on this topic or are (likely non-contended) locks
> not expensive enough to justify the added complexity?
> 
> thanks,
> tim
> 
> 
>>
>>>
>>> Regards,
>>> Arnaud
>>>
>>>>
>>>> in this case, so we re-try once before we really start to sleep
>>>>
>>>> Signed-off-by: Tim Blechmann <tim@klingt.org>
>>>> ---
>>>>   drivers/rpmsg/virtio_rpmsg_bus.c | 24 +++++++++++++++---------
>>>>   1 file changed, 15 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> index 905ac7910c98..2a9d42225e60 100644
>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> @@ -587,21 +587,27 @@ static int rpmsg_send_offchannel_raw(struct
>>>> rpmsg_device *rpdev,
>>>>         /* no free buffer ? wait for one (but bail after 15 seconds) */
>>>>       while (!msg) {
>>>>           /* enable "tx-complete" interrupts, if not already enabled */
>>>>           rpmsg_upref_sleepers(vrp);
>>>>   -        /*
>>>> -         * sleep until a free buffer is available or 15 secs elapse.
>>>> -         * the timeout period is not configurable because there's
>>>> -         * little point in asking drivers to specify that.
>>>> -         * if later this happens to be required, it'd be easy to add.
>>>> -         */
>>>> -        err = wait_event_interruptible_timeout(vrp->sendq,
>>>> -                    (msg = get_a_tx_buf(vrp)),
>>>> -                    msecs_to_jiffies(15000));
>>>> +        /* make sure to retry to grab tx buffer before we start waiting */
>>>> +        msg = get_a_tx_buf(vrp);
>>>> +        if (msg) {
>>>> +            err = 0;
>>>> +        } else {
>>>> +            /*
>>>> +             * sleep until a free buffer is available or 15 secs elapse.
>>>> +             * the timeout period is not configurable because there's
>>>> +             * little point in asking drivers to specify that.
>>>> +             * if later this happens to be required, it'd be easy to add.
>>>> +             */
>>>> +            err = wait_event_interruptible_timeout(vrp->sendq,
>>>> +                        (msg = get_a_tx_buf(vrp)),
>>>> +                        msecs_to_jiffies(15000));
>>>> +        }
>>>>             /* disable "tx-complete" interrupts if we're the last sleeper */
>>>>           rpmsg_downref_sleepers(vrp);
>>>>             /* timeout ? */
>>>>           if (!err) {
>>
>