On Fri, Apr 21 2017, Felipe Balbi wrote: > Hi Jerry, > > Jerry Zhang <zhangjerry@xxxxxxxxxx> writes: >> (reposting this question in its own subject, instead of buried in a patchset) >> >> Android has migrated to using f_fs for the mtp function, but in the >> process we ran into some performance hiccups in comparison to f_mtp >> during bulk transfer of large files. >> >> Allocating memory at boot allowed f_mtp to use large buffers, >> around 250kB. Compared to using a 16 or 32 kB buffer, the larger buffer > > you don't really need to allocate it at boot time :-) but fair > enough. Also, several (say 200) small buffers (PAGE_SIZE -> 4096 bytes) > would probably be better, but read on. > >> is faster by almost 50%. f_fs can't use io sizes that large due >> to memory fragmentation, since there is a kmalloc on each call. > > right > >> One option here would be to add O_DIRECT as a flag to open >> (similar to what is discussed here: >> http://www.makelinux.net/ldd3/chp-15-sect-3). > > We *could* use O_DIRECT, but I'm not sure that's what's causing > issues. I've been considering how to improve f_fs.c myself for the past > few months, but no time to do it yet. > >> Then on each io, we could get_user_pages, vmap them together, > > get_user_pages_fast() + sg_alloc_table_from_pages() would've been the > best. That way, there would be no copy_to/from_user needed, but that's > something that should happen later. There are lower hanging fruits in > f_fs.c IMO. > >> and pass that address into ep_queue (for sync io only, non direct io >> does not change). This would would allow larger io sizes >> to be used and eliminates a user-kernel copy. Having the flag >> allows users to decide which endpoints they choose to use this way. > > Correct :-) > >> Does this sound like something that people want or find acceptable? > > yes, it _is_ definitely acceptable. But f_fs.c needs a cleanup > first. Also, IMHO a better flag to implement would be O_NONBLOCK. Here's > how f_fs.c works today: > > write(ep2, buf, length); > ffs_epfile_write_iter() > ffs_epfile_io() > usb_ep_queue() > wait_for_completion_interruptible() > > That wait_for_completion_interruptible() is what's killing > performance. Each and every read/write waits for the USB side to > complete. It would've been much better to have something like: > > if (flags & O_NONBLOCK) > wait_for_completion_interruptible() We cannot return to user space before the transfer is completed though. > This would help the write() side of things by a long margin. For reads, > what we could do is have a kernel ring buffer with pre-allocated and > pre-queued usb_requests pointing to different pages in this ring > buffer. When a read() comes, instead of queueing the request right > there, we check if there's data in the internal ring buffer, if there > is, we just copy_to_user(), otherwise we either return 0 or return > -EAGAIN (depending on O_NONBLOCK). > > After that, it becomes easy to implement poll() for endpoint files. > > I really think this would give us better result in the end because the > real problem is with that wait_for_completion_interruptible(). If you > put a sniffer on the traffic, I'm sure you're gonna see several SOFs > going by without data just because of the latency of completing the > current request and, finally, returning control to userspace and so on. If waiting is the problem then isn’t it solved by async IO? With it user space can implement double (triple, whatever…) buffering and as soon as one request is completed, the next one becomes active. I prefer non-blocking to aio, but that’s a matter of preference and design of the application. The advantage of async IO is that user space has more control over what reads and writes happen. f_fs doesn’t know the underlying protocol and I can imagine that in some cases that would matter. >> If there's other suggestions for fixing this issue, I'd be willing to help >> implement. > > see above. -- Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving» -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
