Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2876 bytes --]


Hi,

Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>>> can
>>>>> potentially cause a race condition between both calls. This is bad
>>>> because the
>>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>>> This is due
>>>>> to the fact that the usb request fifo looks for the next element and
>>>> only if
>>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>>> If both
>>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>>> kfifo_seek() will return the same usb_request, which will cause a
>>>> race
>>>>> condition.
>>>>>
>>>>> To solve this problem a syncronization mechanism is necessary. In
>>>> this case it
>>>>> is used a spinlock since f_midi_transmit is also called by
>>>> usb_request->complete
>>>>> callback in interrupt context.
>>>>>
>>>>> On benchmarks realized by me, spinlocks were more efficient then
>>>> scheduling
>>>>> the f_midi_transmit tasklet in process context and using a mutex
>>>>> to synchronize. Also it performs better then previous
>>>>> implementation
>>>> that
>>>>> allocated a usb_request for every new transmit made.
>>>>
>>>> behaves better in what way ? Also, previous implementation would not
>>>> suffer from this concurrency problem, right ?
>>>
>>> The spin lock is faster than allocating usb requests all the time,
>>> even if the udc uses da for it.
>> 
>> did you measure ? Is the extra speed really necessary ? How did you
>> benchmark this ?
>
> Yes I did measure and it was not that significant. This is not about
> speed. There was a bug in that approach that I already explained on

you have very confusing statements. When I mentioned that previous code
wouldn't have the need for the spinlock you replied that spinlock was
faster.

When I asked you about benchmarks you reply saying it's not about the
speed.

Make up your mind dude. What are you trying to achieve ?

> that patch, which was approved and applied BTW.

patches can be reverted if we realise we're better off without
them. Don't get cocky, please.

> Any way, this spinlock should've been there since that patch but I
> couldn't really trigger this problem without a stress test.

which tells me you sent me patches without properly testing. How much
time did it take to trigger this ? How did you trigger this situation ?

> So, this patch fixes a bug in the current implementation.

fixes a regression introduced by you, true. I'm trying to figure out if
we're better off without the original patch; to make a good decision I
need to know if the extra "speed" we get from not allocating requests on
demand are really that important.

So, how much faster did you get and is that extra "speed" really
important ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]