From: Felipe Balbi <balbi@kernel.org>
To: Felipe Ferreri Tonello <eu@felipetonello.com>, linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Michal Nazarewicz <mina86@mina86.com>,
        Clemens Ladisch <clemens@ladisch.de>
Subject: Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function
In-Reply-To: <56DD49AC.8040201@felipetonello.com>
References: <1456947640-20673-1-git-send-email-eu@felipetonello.com> <1456947640-20673-3-git-send-email-eu@felipetonello.com> <87twkm676d.fsf@ti.com> <D065B08D-83C3-45F5-840C-C7752A5CCEDD@felipetonello.com> <87h9gikak2.fsf@intel.com> <56DD49AC.8040201@felipetonello.com>
User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/25.0.90.3 (x86_64-pc-linux-gnu)
Date: Tue, 08 Mar 2016 09:37:15 +0200
Message-ID: <87k2ldifo4.fsf@intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3784
Lines: 103

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


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.
>>=20
>> 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 ?

=2D-=20
balbi

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJW3oEsAAoJEIaOsuA1yqREjNcQALSZGU7dYDs2D0VPhN0A0csX
JEk6V/GA4qWsSJCaD3ILlGws69UUnwfRPOx6Ml7GI8PNv/p/ORJ38OY6/TtFw+u4
Q5IW4n3FjbwmG+rSJupZYQk+h6MemLoiX78rcxTMXTDHk2ORbvhwprCMUCRPK3QZ
KlvX29MsvsYAVLIu5UX5069Uq0Pg+twj1A7soZ5niZGqYJmqtc3k85I1E8USi7hx
z0e/Vpc6kO4dnsaDaN/b3i4cwiTWkf+runym+SneKXtrMBjAfe6SI6zRbmPkwH3J
Qlr0NfK+GLfD7CFC3ISIizsXeeDa0Ga9f8iceEadZ24xBgulr3nrpDp4Csp/r0az
2MK4Q1MjzgoMvYnAxnxP3QYVpjMRDO5mLmLFhBUd5rRcUsqA8PRjgfgGQaMZUzJg
+GImzbPZ9qAkmnAKCJKGEouerbHKJSz5RnHdoaKoEOIp1yEMug7IBa5w2my+AGKb
kswXA95TmTNi2ebqMG9mD4gSiANpJE7ujLYQVVAdz/6euLyUIpe5zdIFosur41QN
dRPzUlk8bpFBQqHNhGRVpz0eZKhb3gBge1MN9Wx07DnVMJjGzzp2gdnb/G4lER+z
WHMblHgShorE+jg6E1oNH5KFfVK5tguM0PXs/tRpCcCqzfu+cOC8HVxNKdUnY9y0
NEyRMDk8vlq325/91Wln
=v2vh
-----END PGP SIGNATURE-----
--=-=-=--