usb: gadget: f_fs: O_NONBLOCK waits MAX_SCHEDULE_TIMEOUT
Hi,
I have been looking at an issue where a phone that is the Function FS
host sometimes locks up and causes the function:
static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data
*io_data) in drivers/usb/gadget/function/f_fs.c to timeout after
MAX_SCHEDULE_TIMEOUT jiffies.
We are opening the ffs connection with the O_NONBLOCK option and this
timeout seems to be far too long.
The code causing the wait is here (line 834):
if (unlikely(ret < 0)) {
/* nop */
} else if (unlikely(
wait_for_completion_interruptible(&done))) {
ret = -EINTR;
usb_ep_dequeue(ep->ep, req);
} else {
The function call wait_for_completion_interruptible(&done) equates to:
wait_for_completion_interruptible_timeout(&done, MAX_SCHEDULE_TIMEOUT);
I was wondering what to change to fix this, is it reasonable to use a
very small timeout if it is O_NONBLOCK or is any blocking not allowed?
I would appreciate any thoughts on how this should work?
Thank you,
Jim
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb: gadget: f_fs: O_NONBLOCK waits MAX_SCHEDULE_TIMEOUT
> > FunctionFS is very specific, because read/write operations are directly > translated into USB requests, which are asynchronous, so you cannot use > O_NONBLOCK. > > If you need non-blocking API you can use Asynchronous I/O (AIO). You can > find some examples in kernel sources (tools/usb/ffs-aio-example/). > > Br, > Robert Baldyga > Thank you, that sounds like the best approach. In this case I think perhaps the long wait without any data is an problem with the imx6 Chipidea USB controller. I guess it should suspend and drop the connections if there is no traffic for more than 10ms? Thanks. Jim Baxter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb: gadget: f_fs: O_NONBLOCK waits MAX_SCHEDULE_TIMEOUT
> On Wed, Apr 01, 2015 at 06:29:05PM +0100, Baxter, Jim wrote: >>> >>> FunctionFS is very specific, because read/write operations are directly >>> translated into USB requests, which are asynchronous, so you cannot use >>> O_NONBLOCK. >>> >>> If you need non-blocking API you can use Asynchronous I/O (AIO). You can >>> find some examples in kernel sources (tools/usb/ffs-aio-example/). >>> >>> Br, >>> Robert Baldyga >>> >> >> Thank you, that sounds like the best approach. >> In this case I think perhaps the long wait without any data is an >> problem with the imx6 Chipidea USB controller. > > What's the possible problem? Sorry for the delay in replying, I have been getting some more details with a USB Analyser. The scenario is that the NCM device is enumerating so we see the messages to: SetAddress (1) GetDescriptor (Device) GetDescriptor (StringN) GetDescriptor (Configuration) SetConfiguration (1) GetDescriptor (String iInterface) GetDescriptor (String iInterface) At this point the NCM host sends Writes to the F_FS EP0 but for some reason the host device does not respond and only issues SOF packets for hours. This happens occasionally and is fixed by turning the device off and on again. Unless I am mistaken from a NCM gadget point of view the attached device is working correctly and there is no way to know it has failed, is that correct? > >> >> I guess it should suspend and drop the connections if there is no >> traffic for more than 10ms? >> > > If the Device side NAK host's IN/OUT token continually, the pipe will > not be stopped, the host will send token continually until the application > cancel this request. > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb: gadget: f_fs: O_NONBLOCK waits MAX_SCHEDULE_TIMEOUT
> > FunctionFS is very specific, because read/write operations are > directly translated into USB requests, which are asynchronous, so > you cannot use O_NONBLOCK. > > If you need non-blocking API you can use Asynchronous I/O (AIO). You > can find some examples in kernel sources (tools/usb/ffs-aio-example/). > > Br, > Robert Baldyga > Thank you, that sounds like the best approach. In this case I think perhaps the long wait without any data is an problem with the imx6 Chipidea USB controller. >>> >>> What's the possible problem? >> >> Sorry for the delay in replying, I have been getting some more details with a >> USB Analyser. >> >> The scenario is that the NCM device is enumerating so we see the messages >> to: >> >> SetAddress (1) >> GetDescriptor (Device) >> GetDescriptor (StringN) >> GetDescriptor (Configuration) >> SetConfiguration (1) >> GetDescriptor (String iInterface) >> GetDescriptor (String iInterface) >> >> At this point the NCM host sends Writes to the F_FS EP0 but for some reason >> the host device does not respond and only issues SOF packets for hours. This >> happens occasionally and is fixed by turning the device off and on again. >> >> > > We may find this 'some reason', is it device error or host error? > > Do you have below patch in your code: > > commit 953c66469735aed8d2ada639a72b150f01dae605 > Author: Abbas Raza > Date: Thu Jul 17 19:34:31 2014 +0800 > > usb: chipidea: udc: Disable auto ZLP generation on ep0 > > There are 2 methods for ZLP (zero-length packet) generation: > 1) In software > 2) Automatic generation by device controller > > 1) is implemented in UDC driver and it attaches ZLP to IN packet if >descriptor->size < wLength > 2) can be enabled/disabled by setting ZLT bit in the QH > > Peter Yes I have that patch, could that be a problem? Jim > >> Unless I am mistaken from a NCM gadget point of view the attached device is >> working correctly and there is no way to know it has failed, is that correct? >> >>> I guess it should suspend and drop the connections if there is no traffic for more than 10ms? >>> >>> If the Device side NAK host's IN/OUT token continually, the pipe will >>> not be stopped, the host will send token continually until the >>> application cancel this request. >>> -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V1 1/1] net: cdc_ncm: Fix TX zero padding
Bjørn Mork writes: > > Ouch! Thanks for finding this. This should go to the stable queue as > well. > > Reviewed-by: Bjørn Mork > Do I need to submit this to the stable queue myself? -- Jim -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V1 1/1] net: cdc_ncm: Fix TX zero padding
Bjørn Mork writes: > "Baxter, Jim" writes: > >> >> Do I need to submit this to the stable queue myself? > > No, davem will handle that. > > That is, assuming that you had posted this to netdev in the first > place... Sorry, I just assumed you did without verifying it when I > replied. You will have to repost the patch to the appropriate > recepients. Use > > scripts/get_maintainer.pl drivers/net/usb/cdc_ncm.c > > for that. Feel free to include my Reviewed-by tag. And if you mark the > patch for "stable" somewhere outside the commit message, then davem will > pick that up and add it to his stable queue in patchwork. I don't know > if there is any officially documented way to do this, but a "stable" tag > inside the [] brackets in the subject, or a comment below the --- marker, > usually works fine. > > Note that the "Cc: stable..." method isn't used for netdev patches. See > https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt > > > Bjørn > Thank you, I will resend this to netdev. I had used the command: scripts/get_maintainer.pl drivers/net/usb/cdc_ncm.c --sections so only sent it to the lists in the first section "USB CDC ETHERNET DRIVER". I will remove the --sections in future. Jim -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC V1 1/1] net: cdc_ncm: Reduce memory use when kernel memory low
From: Oliver Neukum (oneu...@suse.com) Sent: Wed, 17 May 2017 09:44:20 +0200 > Am Dienstag, den 16.05.2017, 20:24 +0200 schrieb Bjørn Mork: >> >> I must say that I don't like the additional complexity added here. If >> there are memory issues and you can reduce the buffer size to >> USB_CDC_NCM_NTB_MIN_OUT_SIZE, then why don't you just set a lower tx_max >> buffer size in the first place? >> >> echo 2048 > /sys/class/net/wwan0/cdc_ncm/tx_max >> Hi The issue is we need the higher performance for Mirrorlink to be able to work correctly. The low memory situation only occurs very occasionally and once the kernel has run compaction if this issue occurs again it will be many hours later. > > Hi, > > that would hurt performance across the board though. > Can we trigger memory compactation earlier? > > Regards > Oliver > We initially tried increasing the vm.min_free_kbytes but the value needed to address the problem was around 65536 which then meant some other applications were unable to run due to there not being enough free memory. The i.MX6 based system has 863MB of RAM in total. Regards, Jim -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC V1 1/1] net: cdc_ncm: Reduce memory use when kernel memory low
From: David S. Miller (da...@davemloft.net) Sent: Wed, 17 May 2017 14:18:19 -0400 > > When there isn't memory pressure this will hurt performance of > course. > > It is a quite common paradigm to back down to 0 order memory requests > when higher order ones fail, so this isn't such a bad change from the > perspective. > > However, one negative about it is that when the system is under memory > stress it doesn't help at all to keep attemping high order allocations > when the system hasn't recovered yet. In fact, this can make it > worse. > Hello David, Do you think the patch should be modified to extend the length of time the 0 order memory requests with a time period of 1 minute for example? Or do you feel the patch is not the correct way this should be performed? Best regards, Jim -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC V1 1/1] net: cdc_ncm: Reduce memory use when kernel memory low
From: David S. Miller (da...@davemloft.net)
Sent: Tue, 23 May 2017 11:26:25 -0400
> From: Oliver Neukum
> Date: Tue, 23 May 2017 10:42:48 +0200
>
>>
>> We could use a counter. After the first failure, do it once, after the
>> second twice and so on. And reset the counter as a higher order
>> allocation works. (just bound it somewhere)
>
> So an exponential backoff, that might work.
>
As an idea I have created this patch as an addition to the original patch
in this series.
Would this be acceptable?
At the moment I have capped the value at 10, does anyone think it needs to
be much higher then that?
Regards,
Jim
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index c06d20f..0e40603 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -89,6 +89,8 @@ struct cdc_ncm_stats {
CDC_NCM_SIMPLE_STAT(rx_ntbs),
};
+#define CDC_NCM_LOW_MEM_MAX_CNT 10
+
static int cdc_ncm_get_sset_count(struct net_device __always_unused *netdev,
int sset)
{
switch (sset) {
@@ -,8 +1113,13 @@ struct sk_buff *
/* allocate a new OUT skb */
if (!skb_out) {
- ctx->tx_curr_size = ctx->tx_max;
- skb_out = alloc_skb(ctx->tx_curr_size, GFP_ATOMIC);
+ if (ctx->tx_low_mem_val == 0) {
+ ctx->tx_curr_size = ctx->tx_max;
+ skb_out = alloc_skb(ctx->tx_curr_size, GFP_ATOMIC);
+ if (skb_out == NULL) {
+ ctx->tx_low_mem_max_cnt =
min(ctx->tx_low_mem_max_cnt + 1, CDC_NCM_LOW_MEM_MAX_CNT);
+ ctx->tx_low_mem_val = ctx->tx_low_mem_max_cnt;
+ }
if (skb_out == NULL) {
/* See if a very small allocation is possible.
* We will send this packet immediately and hope
@@ -1127,12 +1134,13 @@ struct sk_buff *
/* No allocation possible so we will abort */
if (skb_out == NULL) {
- if (skb != NULL) {
+ if (skb) {
dev_kfree_skb_any(skb);
dev->net->stats.tx_dropped++;
}
goto exit_no_skb;
}
+ ctx->tx_low_mem_val--;
}
/* fill out the initial 16-bit NTB header */
nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb_out,
sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 5162f38..25a0aed 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -118,6 +118,8 @@ struct cdc_ncm_ctx {
u32 rx_max;
u32 tx_max;
u32 tx_curr_size;
+ u32 tx_low_mem_max_cnt;
+ u32 tx_low_mem_val;
u32 max_datagram_size;
u16 tx_max_datagrams;
u16 tx_remainder;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC V1 1/1] net: cdc_ncm: Reduce memory use when kernel memory low
From: Oliver Neukum (oneu...@suse.com) Sent: Mon, 12 Jun 2017 12:24:07 +0200 > Am Dienstag, den 23.05.2017, 20:06 +0100 schrieb Jim Baxter: >> From: David S. Miller (da...@davemloft.net) >> Sent: Tue, 23 May 2017 11:26:25 -0400 >>> >>> From: Oliver Neukum >>> Date: Tue, 23 May 2017 10:42:48 +0200 >>> We could use a counter. After the first failure, do it once, after the second twice and so on. And reset the counter as a higher order allocation works. (just bound it somewhere) >>> >>> So an exponential backoff, that might work. >>> >> >> As an idea I have created this patch as an addition to the original patch >> in this series. >> >> Would this be acceptable? >> >> At the moment I have capped the value at 10, does anyone think it needs to >> be much higher then that? > > Hi, > > I am working through mail backlog. If I may ask, has this patch proposal > had a result or does something need to be done still? > > Regards > Oliver > Hi, I have not received any response to my additional patch yet. Do you think I should submit it as a second RFC patchset? Regards, Jim -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 1/1] net: cdc_ncm: Reduce memory use when kernel memory low
From: David S. Miller (da...@davemloft.net) Sent: Fri, 30 Jun 2017 12:59:53 -0400 To: jim_bax...@mentor.com Cc: linux-usb@vger.kernel.org, net...@vger.kernel.org, linux-ker...@vger.kernel.org, oli...@neukum.org, bj...@mork.no, david.lai...@aculab.com Subject: Re: [PATCH V2 1/1] net: cdc_ncm: Reduce memory use when kernel memory low > From: Jim Baxter > Date: Wed, 28 Jun 2017 21:35:29 +0100 > >> The CDC-NCM driver can require large amounts of memory to create >> skb's and this can be a problem when the memory becomes fragmented. >> >> This especially affects embedded systems that have constrained >> resources but wish to maximise the throughput of CDC-NCM with 16KiB >> NTB's. >> >> The issue is after running for a while the kernel memory can become >> fragmented and it needs compacting. >> If the NTB allocation is needed before the memory has been compacted >> the atomic allocation can fail which can cause increased latency, >> large re-transmissions or disconnections depending upon the data >> being transmitted at the time. >> This situation occurs for less than a second until the kernel has >> compacted the memory but the failed devices can take a lot longer to >> recover from the failed TX packets. >> >> To ease this temporary situation I modified the CDC-NCM TX path to >> temporarily switch into a reduced memory mode which allocates an NTB >> that will fit into a USB_CDC_NCM_NTB_MIN_OUT_SIZE (default 2048 Bytes) >> sized memory block and only transmit NTB's with a single network frame >> until the memory situation is resolved. >> Each time this issue occurs we wait for an increasing number of >> reduced size allocations before requesting a full size one to not >> put additional pressure on a low memory system. >> >> Once the memory is compacted the CDC-NCM data can resume transmitting >> at the normal tx_max rate once again. >> >> Signed-off-by: Jim Baxter > > If someone could review this patch, I remember this issue being discussed > a while ago, I would really appreciate it. > Hello, For reference this replaces the original discussion in http://patchwork.ozlabs.org/patch/763100/ and http://patchwork.ozlabs.org/patch/766181/ Best regards, Jim -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 1/1] net: cdc_ncm: Reduce memory use when kernel memory low
> Jim Baxter writes: > >> The CDC-NCM driver can require large amounts of memory to create >> skb's and this can be a problem when the memory becomes fragmented. >> >> This especially affects embedded systems that have constrained >> resources but wish to maximise the throughput of CDC-NCM with 16KiB >> NTB's. >> >> The issue is after running for a while the kernel memory can become >> fragmented and it needs compacting. >> If the NTB allocation is needed before the memory has been compacted >> the atomic allocation can fail which can cause increased latency, >> large re-transmissions or disconnections depending upon the data >> being transmitted at the time. >> This situation occurs for less than a second until the kernel has >> compacted the memory but the failed devices can take a lot longer to >> recover from the failed TX packets. >> >> To ease this temporary situation I modified the CDC-NCM TX path to >> temporarily switch into a reduced memory mode which allocates an NTB >> that will fit into a USB_CDC_NCM_NTB_MIN_OUT_SIZE (default 2048 Bytes) >> sized memory block and only transmit NTB's with a single network frame >> until the memory situation is resolved. >> Each time this issue occurs we wait for an increasing number of >> reduced size allocations before requesting a full size one to not >> put additional pressure on a low memory system. >> >> Once the memory is compacted the CDC-NCM data can resume transmitting >> at the normal tx_max rate once again. >> >> Signed-off-by: Jim Baxter > > This looks good to me. > > I would still be happier if we didn't need something like this, but I > understand that we do. And this patch looks as clean as it can get. I > haven't tested the patch under any sort of memory pressure, but I did a > basic runtime test on a single MBIM device. As expected, I did not > notice any changes with this patch applied. > > But regarding noticable effects: The patch adds no printks, counters or > sysfs attributes which could tell the user that the initial buffer > allocation has failed. Maybe add some sort of debug helper(s) in a > followup patch? How did you verify the patch operation while testing it? > > Anyway, that's no show stopper of course. So FWIW: > > Reviewed-by: Bjørn Mork > Hello Bjørn, I tested this with printk's to show when the low memory code was triggered and the value of ctx->tx_low_mem_val and ctx->tx_low_mem_max_cnt. I created a workqueue that slowly used up the atomic memory until the code is triggered. I could add debug prints, though I have noticed that cdc_ncm_fill_tx_frame() does not currently have any debug prints do you think this is because it can be called in an atomic context and I think debug messages if enabled could cause too great a delay? Regards, Jim -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
