Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1277627 > unrolled thread

Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests Felipe Ferreri Tonello <eu@felipetonello.com> - 2015-11-25 18:30 +0100
    Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests Clemens Ladisch <clemens@ladisch.de> - 2015-11-27 10:10 +0100
      Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests Felipe Ferreri Tonello <eu@felipetonello.com> - 2015-11-27 19:10 +0100
        Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests Clemens Ladisch <clemens@ladisch.de> - 2015-11-27 21:00 +0100

#1277627 — Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

[Multipart message — attachments visible in raw view] — view raw

Hi Clemens

On 13/11/15 08:55, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This patch introduces pre-allocation of IN endpoint USB requests. This
>> improves on latency (requires no usb request allocation on transmit) and avoid
>> several potential probles on allocating too many usb requests (which involves
>> DMA pool allocation problems).
>>
>> This implementation also handles better multiple MIDI Gadget ports, always
>> processing the last processed MIDI substream if the last USB request wasn't
>> enought to handle the whole stream.
> 
>> ...
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -88,6 +89,9 @@ struct f_midi {
>>  	int index;
>>  	char *id;
>>  	unsigned int buflen, qlen;
>> +	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>> +	unsigned int in_req_num;
>> +	unsigned int in_last_port;
> 
> As far as I can see, in_req_num must always have the same value as qlen.

Yes, I've removed in_req_num.

> 
>> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
>> +	while (!kfifo_is_empty(&midi->in_req_fifo)) {
>> +		...
>> +		len = kfifo_get(&midi->in_req_fifo, &req);
>> +		if (len == 1)
>> +			free_ep_req(midi->in_ep, req);
>> +		else
>> +			ERROR(midi, "%s couldn't free usb request: something went wrong with kfifo\n",
>> +			      midi->in_ep->name);
>> +	}
> 
> kfifo_get() already checks for the FIFO being empty, so you can just
> drop kfifi_is_empty().

Ok. I've simplified this code. I wasn't really happy with it either.

> 
>> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request *req,
>> ...
>> +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.

> 
> This code shows the error message for case 1), but does the action for
> case 2).
> 
> 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.

Agree. It would be useful for users to know about case 1), but like you
said it is probably not worthwhile to do to so.

> 
>> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>> +	if (kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL))
>> +		goto setup_fail;
> 
> The setup_fail code expects an error code in the status variable.

Done.

Felipe

[toc] | [next] | [standalone]

#1278657

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.

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.)


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.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278914

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1278938

Felipe Ferreri Tonello wrote:
> 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).

f_midi_transmit() always fills up the USB packet as much as possible, so
the number of MIDI messages per request will increase automatically when
the ALSA buffer fills up faster that it is emptied by f_midi.

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

Yes.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web