Message-ID: <56589AFD.6010405@felipetonello.com>
Date:	Fri, 27 Nov 2015 18:03:41 +0000
From:	Felipe Ferreri Tonello <eu@...ipetonello.com>
To:	Clemens Ladisch <clemens@...isch.de>, linux-usb@...r.kernel.org
Cc:	linux-kernel@...r.kernel.org, Felipe Balbi <balbi@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Robert Baldyga <r.baldyga@...sung.com>
Subject: Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

Hi Clemens

On 27/11/15 09:05, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 13/11/15 08:55, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
>>>> +static void f_midi_transmit(struct f_midi *midi)
>>>> +{
>>>> +...
>>>> +		len = kfifo_peek(&midi->in_req_fifo, &req);
>>>> +		...
>>>> +		if (req->length > 0) {
>>>> +			WARNING(midi, "%s: All USB requests have been used. Current queue size "
>>>> +				"is %u, consider increasing it.\n", __func__, midi->in_req_num);
>>>> +			goto drop_out;
>>>> +		}
>>>
>>> There are two cases where the in_req FIFO might overflow:
>>> 1) the gadget is trying to send too much data at once; or
>>> 2) the host does not bother to read any of the data.
>>>
>>> In case 1), the appropriate action would be to do nothing, so that the
>>> remaining data is sent after some currently queued packets have been
>>> transmitted.  In case 2), the appropriate action would be to drop the
>>> data (even better, the _oldest_ data), and spamming the log with error
>>> messages would not help.
>>
>> True. In this case the log will be spammed.
>>
>> How would you suggest to drop the oldest data? That doesn't really seem
>> to be feasible.
> 
> There is usb_ep_dequeue().  Its documentation warns about some hardware,
> but it would be possible to at least try it.
> 
>>> I'm not quite sure if trying to detect which of these cases we have is
>>> possible, or worthwhile.  Anyway, with a packet size of 64, the queue
>>> size would be 32*64 = 2KB, which should be enough for everyone.  So I
>>> propose to ignore case 1), and to drop the error message.
> 
> After some thought, I'm not so sure anymore -- the ability to buffer
> more than 2 KB of data is part of the snd_rawmidi_write() API, so this
> could introduce a regression.  And I can imagine cases where one would
> actually want to transmit large amounts data.

One thing to consider is that the ALSA rawmidi device buffer is
sequential and our USB request buffer is not. This means that our 32
(qlen) * 256 (buflen) = 8KB of data is non-linear. Some requests might
have 3 or 4 bytes (average size of a normal MIDI message) of data and
some others might contain the full 256 bytes (for SysEx messages).

I am considering this especially for MPE (Multidimensional Polyphonic
Expression) MIDI protocol. On few benchmarks I did, a device that
implements this protocol generates around 500-2000 b/s of *raw* MIDI
data. And in practice only 4 (average MIDI message) * 32 (USB requests
defined by qlen) bytes will be used. Which means that the 8KB USB
request buffer will be under used.

So I think we have to treat the ALSA buffers and the USB request buffers
differently.

That's why I think this approach is fine by allowing the user to
increase that number of requests and its size if it needs to deal with a
higher throughput devices.

> 
> I think the safest approach would be to behave similar to the old driver,
> i.e., when the queue overflows, do nothing (not even dropping data), and
> rely on the transmit completion handler to continue.  (This implies that
> ALSA's buffer can fill up, and that snd_rawmidi_write() can block.)
> 

The previous implementation would not block, even though
snd_rawmidi_write() can block, because it was been created a new USB
request for each write call and data was been consumed even if this
request would not be enqueued to the endpoint.

But, anyway, I agree with your suggestion.

> 
> It you want to dequeue outdated data, I think this should be done with
> a timeout, i.e., when the host did not read anything for some tens of
> milliseconds or so.  This would be independent of the fill level of the
> queue, and could be done either for individual packets, or just on the
> entire endpoint queue.

That can be done. But I believe in another patch since it is not
required to work for this patch.

== Conclusion ==

Based on our conversation and your suggestions, I think that to just
ignore if an overrun occurs to the USB requests is fine. Upon completion
the request will be reused.
Important to note that if the overrun occurs, it will cause user-space
to block until a) the completion function is called successfully or b)
snd_rawmidi_write() times out. Which I think this is expected by ALSA users.

Does that make sense?

If yes then I will send the v6 of this patch.

Felipe

Download attachment "0x92698E6A.asc" of type "application/pgp-keys" (7196 bytes)

Powered by blists - more mailing lists