Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161218AbbKFKFN (ORCPT ); Fri, 6 Nov 2015 05:05:13 -0500 Received: from mail-bn1bbn0105.outbound.protection.outlook.com ([157.56.111.105]:52768 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1032433AbbKFKFF (ORCPT ); Fri, 6 Nov 2015 05:05:05 -0500 Authentication-Results: spf=fail (sender IP is 192.88.158.2) smtp.mailfrom=freescale.com; vger.kernel.org; dkim=none (message not signed) header.d=none;vger.kernel.org; dmarc=none action=none header.from=freescale.com; Date: Fri, 6 Nov 2015 17:48:45 +0800 From: Peter Chen To: Robert Baldyga CC: , , , , , , Subject: Re: [PATCH 03/23] usb: gadget: f_sourcesink: free requests in sourcesink_disable() Message-ID: <20151106094844.GA2809@shlinux2> References: <1446555242-3733-1-git-send-email-r.baldyga@samsung.com> <1446555242-3733-4-git-send-email-r.baldyga@samsung.com> <20151106081512.GD12560@shlinux2> <563C69C3.2030102@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <563C69C3.2030102@samsung.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-EOPAttributedMessage: 0 X-Microsoft-Exchange-Diagnostics: 1;BY2FFO11FD049;1:K5M2B48gFT2YA1RZgqmfZJ73aIRiRvMR/Jr2G/F0l8Saw48uRKnytKVdpqx2iHa4NR980Kzoapj22KWDQptDv6WsrSue3NARenqzCzM73goYfZB5g/cdlPqbIvJLu5TGmmELVh20bgEgPpAeYwxEJTignkLIR6FpvrnhDezmcT61p+LNTDtsSFkYaVkKi6J7CWL4jVriSAmFTkO941mWGItNGd1NnOZsILkOOQIimvrMAFiQc/njs/Y02MCQ/LED+CWmuYTBs8a6BV2fYVSLo2urfP71cr3Ok6dQMEwls7JA2YDzMR/lU+ZqA8OhO1nshuNqtem/LmKnSuzKhVywtstEYu4rsMLpviG0Pwgiu5BurhVVyO3gGM8yZvYIIH6l X-Forefront-Antispam-Report: CIP:192.88.158.2;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10019020)(6009001)(2980300002)(1109001)(1110001)(339900001)(189002)(199003)(24454002)(479174004)(377454003)(104016004)(33716001)(81156007)(77096005)(6806005)(5008740100001)(50466002)(85426001)(86362001)(11100500001)(19580405001)(19580395003)(83506001)(2950100001)(54356999)(4001350100001)(50986999)(97736004)(76176999)(5007970100001)(87936001)(5001960100002)(97756001)(110136002)(33656002)(15975445007)(105606002)(106466001)(189998001)(46406003)(47776003)(92566002)(23726002)(93886004)(69596002);DIR:OUT;SFP:1102;SCL:1;SRVR:BN1PR0301MB0627;H:az84smr01.freescale.net;FPR:;SPF:Fail;PTR:InfoDomainNonexistent;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BN1PR0301MB0627;2:anWiCj3ZPLOXP1SVPm+6uWatU5XVRV9z0PXkAGg0eUR2l2oU5rQ/x7KkBXs73NVf5Ag3Yv0sQTWkAfY2mYMOngP1pj2s/ge5e+JNUlQppkT1NnBPut/d4IcjvKOSSIQ40b0obTRdyVtvl9mFaHrL25cTswc4DvXiHvcUIwTZ1cs=;3:tUODN/uJwQAM0eNRJ2PR1KpjHKTZChpaF+3r7k73XqXVRRhiM5UgijyzNZOB7y4AiWsIM4VHlrbSvv6AfJEN6ot5rVjw4mOocBuf3cgJRW7aoeGn/9wb8ghxbz4S1CcUBF/zp1Y40CNheG8OEeL99w46thshfeNG6qVJHi30sqlLx+vxxFmgDVmaLFqyq8Wq+XZ12PfSeXJpRrKgzceURQimrXeL5dg15JgEzcmpp6CVj8npHh+PxsU/E1gO+qHIzREnAipSfM1XfM0ZrkyfYA==;25:JL0WxiZEKo5/MpDr+aWoWumQe5zz7FE+Kfi1qYN0j+qZpe15XM0tI89ryRl9Jxkh9e/8x4nRCKbA86NU7htXxyoi/bz0iSKrH/A7peZLmuE2J0oWGhpT71fLL1+vqsxAcoMTbdYj4ONMu7Jompyb1OsxCSZFYHMo3p0nfEQrWEQjwmpElzcQLoIg/CjSi5zgpaqjiakfCLOHzm551nz95Mr+MGXXbMBWk4vkWqGfMYERXiS5YDBPN3NmIDE+oJRhPvMG4VE5hluGcXCheyEVCQ== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(42134001)(42139001);SRVR:BN1PR0301MB0627; X-Microsoft-Exchange-Diagnostics: 1;BN1PR0301MB0627;20:cmRk+LJF440OLHNpKmioyFb9LzrZ95xx4Gm6RTQN3qVJYIX9DUB5SCm4e+yxiOxhbgU1NoWDgxoPHIdsQ4V0SDecp8cFZt2HqND3/7qeokZzU7FYJI08gu/CvJsUADLtfq0N79sLkY35oKlPf3LFsx9xkI49ojt+4WENrCuSckg0GhY+zldRQDvLFIjBKKV4iRpvHChGcxL3bv4jpeDBj2ruxq0qwXqI4NoGlE5+9AMPm66JN/6fs/zHijWbxt7MCz9vnT0lExovxs/JcTxPLyJn0vMz/6M264gBFf6GeYR9pGHY1+VmwWfn5TMaDsEjmqWDEv799+n6Nr9Xa8niMmW+YEh76u2D06dlWYbPIio=;4:+M3XhUHU5SS/eRPk9ly8oVqShmSDBxbwnL3AlmWkfz4GpJJUoPURsZfIRsqUkCW5IhHhmltHRccNXytSNbbY+oQQzRlapHTT7ZRv3lM15JOgVPhVeblWoyNJ3jdbyAFByzO6lXIQvjb3wfzVtu7roRVuKSFR1V/YnvBN3RHMnBFQNyLDJmr3y2nGXOnEgcYX7HO+JeWOQ6JcqReSB937kCj/fOhnKo01+diH+6AAUWNgj4TedT5jxK279X8L3XnbOPYdZK6Dv49Bu5t0MzgvWqXLM/rDsf896NUPlW5aMgayWj/1pEbcW+Yx97w3qvYmfC88Dj0O/9fKEG3s1haYBmRDwePMXE5dFb18dHrX6kpGjyFQlx8xVlxQGIX9uOTS X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(520078)(8121501046)(5005006)(10201501046)(3002001);SRVR:BN1PR0301MB0627;BCL:0;PCL:0;RULEID:;SRVR:BN1PR0301MB0627; X-Forefront-PRVS: 07521929C1 X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BN1PR0301MB0627;23:I5OmJSk/R9vJbP5BcXF34SF5I+Fscsl/hLg38xV?= =?us-ascii?Q?rSzMQQ7c+rXgrmfksylLTulnZPXxPkN2QnIdvkau3puzt0ClViH6oOfHWFH5?= =?us-ascii?Q?7exVXNFInq+sdUtETeaQ6CCmTAwD7X2wroY80lr0LpNOsQNYFZIwdRZsMAZi?= =?us-ascii?Q?Ijpf8hnEXe2TtZMCFtOa6H1vLeF/8qP3BB6XXC0F6Oj3gsn5tfOJ68PJymzx?= =?us-ascii?Q?D4r7bqBsC7Q5erFXyaEI7mVjhyosxLtP62E7fOnDd1bJm21qp7WiyIP5+5Gz?= =?us-ascii?Q?76ix5r6fiHKfUebL0vFvUZcClgVbxoTrcsW6eo0H5AZSjLT6BJl0VzC8PoXr?= =?us-ascii?Q?YAGMMrQP1fBVhbxaYAEe30Wj1CMP7FSDMePZeWB2iASKwyIoAFbKLlFP5YvD?= =?us-ascii?Q?6rDxnOPlyCR648MS/FPlZ6v4UmudnlVFCzNDuusN64qf7NlGjyWHFylIoR4Z?= =?us-ascii?Q?ILB+BHroa9KKcaGRJpfbxKRutmWpuy0tXTYjhWGKxm3JcXBMecE24B4UEyVh?= =?us-ascii?Q?KiHz9RuiRECvMiTTIkffCc1Xpjx/6CN0KgM6fPfcwJwnec/z7EHYMAoEb+tQ?= =?us-ascii?Q?1PXHCK+MWoeIExzK6B1LKtEGrmE38xy/AZdtJJ4mYIO/IDiMxMiWsGHlsPWW?= =?us-ascii?Q?KSU/qex/wpvU8crumtY4yvRTPPKG4aRJqKfYh7BrP4pnuig1f6HFHI/bRLU9?= =?us-ascii?Q?gAAGBgMF68wKXpzMD5RnjLpSWuo+61st3zc88gtdzw2ZJa1kqiXzSIJES6KJ?= =?us-ascii?Q?2MT5c92XHND84I6R0G31xfHbztrNx9SqOX3/esK70bj6d2tYZHhQOcSQUHS/?= =?us-ascii?Q?OuLL2Fg7evOcQo4Mr871d7yIeUl6WBpOK5jOQs7Zs0yIWvuauxYk6w1bA9/a?= =?us-ascii?Q?ThtIVDEE2O1/g/+inDC7QCt89WlhKyX8qJVmh3dBzCx6e4l2XdaW59otrcaW?= =?us-ascii?Q?lctXD3/bQderVCSE+iv08yChZ6GH/BRYIu38Lc4mdNjWEn+ds+OAOsENDpzm?= =?us-ascii?Q?TW1YlMzhuMCuz8v0iF+PwIme4+65dczescs1d3SJ6x6Hr7cGqsW+Zo7qXkMo?= =?us-ascii?Q?Nnto+JfqDqkK8S4x6P9/y3IpjhkRD2wC+IsgacV/I3L8DH2lsJjCkrKB+xYz?= =?us-ascii?Q?SljqyEyLDJSHU16S2nvTWyDya4EIVh8JVWGPeR05hVoP1wDCNbfFCEnUegsT?= =?us-ascii?Q?uY9r7HustV5mcnMV8q8z5gox+8Gdu752A4ZNQ?= X-Microsoft-Exchange-Diagnostics: 1;BN1PR0301MB0627;5:S5wQhLkmp/buXFgGbFVLP70ufDm5NkVWU71FYJefpkpIm7eLnqlPc7fJ+1VhpYViKkhL33dhRPybbsdMIC32ctuPRop0kGw6WoSk8TnWBNJsTLGJsG5OUStoYtMO2va+IwLsRuET3XiwosX0LVuZFw==;24:rdZfQPG8SDQWzz/3t5uw18Kth03gC6y8ljjpzhACZAYmQPTSAIFZHfDza20a6RqRcuuXK+5iIQIim3xyF3yhiVtX/g/CbUSGFDhXhbMy2AI=;20:UDE1R8+u+aLrFFbGG//3Vy66dyBmEV2i6bVnCNUjmN4trBMNbvhU36SKKjSlkcth2UhBysEr1+sO3EEogpcqYQ== SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Nov 2015 09:50:49.3472 (UTC) X-MS-Exchange-CrossTenant-Id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=710a03f5-10f6-4d38-9ff4-a80b81da590d;Ip=[192.88.158.2];Helo=[az84smr01.freescale.net] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN1PR0301MB0627 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4694 Lines: 135 On Fri, Nov 06, 2015 at 09:50:11AM +0100, Robert Baldyga wrote: > On 11/06/2015 09:15 AM, Peter Chen wrote: > > On Tue, Nov 03, 2015 at 01:53:42PM +0100, Robert Baldyga wrote: > >> USB requests in SourceSink function are allocated in sourcesink_get_alt() > >> function, so we prefer to free them rather in sourcesink_disable() than > >> in source_sink_complete() when request is completed with error. It provides > >> better symetry in resource management and improves code readability. > >> > >> Signed-off-by: Robert Baldyga > >> --- > >> drivers/usb/gadget/function/f_sourcesink.c | 33 +++++++++++++++++++++++++++--- > >> 1 file changed, 30 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c > >> index a8b68c6..d8f5f56 100644 > >> --- a/drivers/usb/gadget/function/f_sourcesink.c > >> +++ b/drivers/usb/gadget/function/f_sourcesink.c > >> @@ -22,6 +22,8 @@ > >> #include "g_zero.h" > >> #include "u_f.h" > >> > >> +#define REQ_CNT 8 > >> + > > > > It would be buffer if we can have module parameter for this like > > qlen at f_loopback. > >> @@ -561,7 +568,6 @@ static void source_sink_complete(struct usb_ep *ep, struct usb_request *req) > >> req->actual, req->length); > >> if (ep == ss->out_ep) > >> check_read_data(ss, req); > >> - free_ep_req(ep, req); > > > > I find the current code may cause memory leak, since above code > > is only called one time. > > > > I don't see what you mean. Can you explain a bit more in which situation > can memory leak take place? Oh, sorry, I have not seen there is a "break;" at the source_sink_start_ep for none-iso endpoint, so, I am wrong, no memory leak here. I have tried changing bulk for 8 requests, the performance improves greatly, but still a little problem for OUT, I will see what's the matter. Besides, it will be better we can have a qlen parameters like f_loopback, in that case, we can improve the performance for gadget, and get the best performance when testing with usbtest, I will do it later. Peter > > I've only changed place where requests are freed. Now they are freed in > sourcesink_disable(). The current code is even more memory leak > resistant, because requests will be freed even in case of failure of > usb_ep_queue() in source_sink_complete(), which was not provided so far. > > >> return; > >> > >> case -EOVERFLOW: /* buffer overrun on read means that > >> @@ -613,7 +619,7 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in, > >> ep = is_in ? ss->in_ep : ss->out_ep; > >> } > >> > >> - for (i = 0; i < 8; i++) { > >> + for (i = 0; i < REQ_CNT; i++) { > >> req = ss_alloc_ep_req(ep, size); > >> if (!req) > >> return -ENOMEM; > >> @@ -635,8 +641,18 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in, > >> free_ep_req(ep, req); > >> } > >> > >> - if (!is_iso) > >> + if (is_iso) { > >> + if (is_in) > >> + ss->iso_in_req[i] = req; > >> + else > >> + ss->iso_out_req[i] = req; > >> + } else { > >> + if (is_in) > >> + ss->in_req = req; > >> + else > >> + ss->out_req = req; > > > > The req will be different for both bulk and iso, why you only > > have array for iso requests? > > Because only for iso endpoints there is allocated more than one request. > I don't know why, but I've decided to not change this. > > > > >> break; > >> + } > >> } > >> > >> return status; > >> @@ -764,8 +780,19 @@ static int sourcesink_get_alt(struct usb_function *f, unsigned intf) > >> static void sourcesink_disable(struct usb_function *f) > >> { > >> struct f_sourcesink *ss = func_to_ss(f); > >> + int i; > >> > >> disable_source_sink(ss); > >> + > >> + free_ep_req(ss->in_ep, ss->in_req); > >> + free_ep_req(ss->out_ep, ss->out_req); > >> + > >> + if (ss->iso_in_ep) { > >> + for (i = 0; i < REQ_CNT; i++) { > >> + free_ep_req(ss->iso_in_ep, ss->iso_in_req[i]); > >> + free_ep_req(ss->iso_out_ep, ss->iso_out_req[i]); > >> + } > >> + } > >> } > >> > >> /*-------------------------------------------------------------------------*/ > >> -- > >> 1.9.1 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- Best Regards, Peter Chen -- 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/