From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753736AbcDFVwB (ORCPT ); Wed, 6 Apr 2016 17:52:01 -0400 Received: from mail-bn1bon0140.outbound.protection.outlook.com ([157.56.111.140]:60976 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753352AbcDFVv7 (ORCPT ); Wed, 6 Apr 2016 17:51:59 -0400 Authentication-Results: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=hpe.com; Message-ID: <570584F1.10909@hpe.com> Date: Wed, 6 Apr 2016 17:51:45 -0400 From: Waiman Long User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130109 Thunderbird/10.0.12 MIME-Version: 1.0 To: Tejun Heo CC: "Theodore Ts'o" , Andreas Dilger , Christoph Lameter , , , Scott J Norton , Douglas Hatch , Toshimitsu Kani Subject: Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper functions References: <1459566578-30221-1-git-send-email-Waiman.Long@hpe.com> <1459566578-30221-3-git-send-email-Waiman.Long@hpe.com> <20160404160228.GW7822@mtj.duckdns.org> In-Reply-To: <20160404160228.GW7822@mtj.duckdns.org> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [72.71.243.60] X-ClientProxiedBy: BY1PR17CA0034.namprd17.prod.outlook.com (10.162.18.172) To TU4PR84MB0320.NAMPRD84.PROD.OUTLOOK.COM (10.162.186.30) X-MS-Office365-Filtering-Correlation-Id: d444a5d6-c209-400e-08fb-08d35e65ab6d X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0320;2:4worvqDKNSpOlL1g3FRdEPYgEVgHgh/SokgGS598hvTK93lFlAqvpoz+OxXBrcVXIyo2D2Sd3c6XFbnTXo8gzwx82iRDaqqSXXBVxrfMM6z0if6LVTWqkmT0GKqJuzXxEjHazq7I17Rz5zuum1Wd++0c0CMG3wyeyEZMFPdwG50XHnXIdP+z46RuLI6F/9aQ;3:PG89vz8Y5ihndPVRL7xINHkh6o6S2d+/XzIStmTqRN0+gkAgVM146IduMS69sY6aIi+XQ6zMoI2Ni8/y/Cuyi/R0mrMcdEkc3OBvIIjGS9NmO/vnHPJsHamHz2QHEA9a;25:H/3X1Vt38JpqH0ltP1nM0gBLlIBa1UWKwf196fj/MlMzoyZNqFSFaDtAAdf1EYAIbg0KUa4QnJHx6XPpcLsbDP8L2R6LDQh7EEKws7ffSJCj+Me/2lkIkG/MsMzbg4yWkSIfFc9vROalMcvNmPJSe3O4IVc8t/dGn2aIGTzcFtk3RtZ3RnwVx55hzpGN5p2BsTM5sVZyJuDZQN+rj+xGvUwR1NvoOtfUWFZZCnFGdWJUy389sRUpsWbOoBNHcaZWhOcgXsKvKb0MRQvh2QBjJVwLPstlUkI7wXhXNnCZNGvOaqJPWiDwVwTzrbB6qsz31vuXa4EZ0vOJn+fZY6IOqw== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0320; X-LD-Processed: 105b2061-b669-4b31-92ac-24d304d195dc,ExtAddr X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0320;20:X7x9cucjimedpBbx1ywBlNBG1YmLb+PrPMVogvww82eGOpiHo/JR9Bw9kq+8b3DM1LA7tafeXkbJICEf9ClzN+0AR2wXl0BEacywUiojuL0obHxXfdpfWvk0P0ZOXbsIkEOjr0moXutJX0sQ3dgXf9m3Saf4HUISaFziOjnzliaS5rYvUPBkG6i1eb2t0p2bjuDEJqHbe05dQbKuLu0wwJPbCd4rLYkkLvs45YWwI0n/3JQC5FYN2B7GiRmV2/Wgu+gH/SGWfak0gtDM/Byxxd3XnfF7hP+g8sGJv/rCV7yRkqoOypLftX40quFtRuVUGSzXOIAcEPsbkYkA+RjZxQ==;4:5Z2e4QAssI4RxN9UdGoz+9pwKYAfk4+siF9xJ/vQgYLBs8XrOZLGdqxBVJNdtU489J7r97EPNYXO4PEAdyAz1s9YjvVdN44ZnSBX12u+NdK4AiCYuoNmITpfvIvimqg7L0xCq1nsiByKIPQ2/Nzmb/eSzcKp5EUCWGEBy4pggjSXdsmxEIwoUw8vfXwWwxoXT2+icfSMOiIb19zhU0gtYiyNCz+RlpLog9SgelKLkP3B3/7RTYHoUMWfO8CNnb78LQbuuFhPRahmqJQZ1rE3EfYFFsQQhEpMg6CEx5dMbJCr2wYAHomEjAQ4wVnuaCOwtZm0qQtA+ayULDH1oxo0gtTnBZcg1sEeAbxaG0cqNvwhriELieTlXQ8G36aZIukSbG+emUM7j/amOwpf3Uwfaw== X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6055026);SRVR:TU4PR84MB0320;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0320; X-Forefront-PRVS: 0904004ECB X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6049001)(6009001)(377454003)(24454002)(33656002)(81166005)(54356999)(2906002)(65956001)(189998001)(66066001)(47776003)(77096005)(4326007)(76176999)(117156001)(50986999)(2950100001)(64126003)(50466002)(65816999)(92566002)(5008740100001)(23756003)(6116002)(42186005)(65806001)(86362001)(230700001)(110136002)(4001350100001)(36756003)(586003)(83506001)(1096002)(5004730100002)(3846002);DIR:OUT;SFP:1102;SCL:1;SRVR:TU4PR84MB0320;H:[192.168.142.152];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;TU4PR84MB0320;23:58QN4zyHlCsdARE64bAIQJwpt6F9v4+oQZD/1HQ?= =?iso-8859-1?Q?o1qQI1xYuCOcHLR4aKjXOvnUmIe6d6Pi4PL1moaHU1axnSvqDpqz1GFvLi?= =?iso-8859-1?Q?tHu6PLlxGgHjI5fT8xNR14M7cCKJXuUqOPGC0J7UQL6KbO6ROP1FlQ4wj4?= =?iso-8859-1?Q?Qk4dFsuY6pjykGbwspghWNyTEDq6C/2ucaAFrKPKo4y1wSgYRUPQkbCBTn?= =?iso-8859-1?Q?P0w3aKbNt6iKfMvZwL3eDM/Um2qrfh2kjVxFeVIHSSgnPr/q5LDqtjccdP?= =?iso-8859-1?Q?t2SCbBR02DW/qt9l3vxVX0HVb5xmQDcHD8T64cx6FPlPR/TCL0AISJ3Fm4?= =?iso-8859-1?Q?HPUeMr3wM69SlfSnctXzZYmTslQaq1WIOiNa1aQUcRD7iXk4pholOdxKp5?= =?iso-8859-1?Q?enqvq6qAi/3kOVHISLqBLJLhrXS5fmqJFG7EzY36KeBqnj9DRH1v98T5UF?= =?iso-8859-1?Q?g/ErF20lYoKEQkGiCOetbDfaW5ENZUyDqTLlC0PGD6LdyXAt7HCcs0ZLfo?= =?iso-8859-1?Q?iNKDDdyQUjnngyzqlEqXa6OyHwUUZUnbGULVXzc6KiwEqiOEPeli6IbpPU?= =?iso-8859-1?Q?DDA8ba1iKqIswxuv2DohJriKMSb5X4ek3NeD16mKo35xVLqOkSgvZaVuP1?= =?iso-8859-1?Q?xD/fnCTi2c5+DAa9+aYLFSkgj5XwOrDHhEyIuqlRc6tNcmpgrmdOfgLUC8?= =?iso-8859-1?Q?VfsKBnw8kzphLVRJjbJClh7H0i5N4xld+mTelPnLBC6z5EMeZf4a3czSyb?= =?iso-8859-1?Q?IM4dscms+1oa6M3HzIZbeTh3WMwdd9DPWhGUI8eVuulwKK4QjqVSsvWPAa?= =?iso-8859-1?Q?WJujggDdi5lH1R3yDTlj78oS3OrWG8103P+BxKulsX68xszPSSkzd3wiH2?= =?iso-8859-1?Q?XdXo7CKKmZi4E2tRg1OxLX5SztvgAnFsRVNt4uFn23FC8uR20JDPneu+cz?= =?iso-8859-1?Q?B8gb9iCVN20YlzgX60ht7EucNGEnGd+JTo7GMsVfArjxl14R7n7VxBfjkS?= =?iso-8859-1?Q?bD/jAoazuyYYpwsvYLFt49kNjExbwrxBrP1gqOMzpDjBVn3UuLHQ4jyMU?= X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0320;5:6ANyhIwAAi6BZRiRgPTksGSUnvdckAf93Ynu6ReSBwNPk01Yg68pkqXGXanbo47ToFQ0RdYeAoPbBWuYI5GrjcB2Da1w8TwNkvpjQWnR3WZLjt7qDUSv6o/+qlFTxFiU10nvj7nueD4rf3ILJ65ukA==;24:rXf+S/O4u/FUVZW2MGhbkctP5dbCsAVvv8JOb8d5qavjlO/7vaQvRiGcaqm48T1BeQVgkwDxvVnvN67FRgjo9yOsyo+O63LB/YkH6eVxM+o= SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Apr 2016 21:51:53.5128 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: TU4PR84MB0320 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/04/2016 12:02 PM, Tejun Heo wrote: > Hello, > > On Fri, Apr 01, 2016 at 11:09:37PM -0400, Waiman Long wrote: > ... >> +struct percpu_stats { >> + unsigned long __percpu *stats; > I'm not sure ulong is the best choice here. Atomic reads on 32bit are > nice but people often need 64bit counters for stats. It probably is a > better idea to use u64_stats_sync. Got that, will incorporate 64-bit counter support for 32-bit architecture. >> +/* >> + * Reset the all statistics counts to 0 in the percpu_stats structure > Proper function description please. Sure. Will do that for all the functions. >> + */ >> +static inline void percpu_stats_reset(struct percpu_stats *pcs) > Why is this function inline? It doesn't need to be inlined, but I need to add a lib/percpu_stats.c file to hold the function which I will do in my v2 patch. > >> +{ >> + int cpu; >> + >> + for_each_possible_cpu(cpu) { >> + unsigned long *pstats = per_cpu_ptr(pcs->stats, cpu); > ^^ >> + int stat; >> + >> + for (stat = 0; stat< pcs->nstats; stat++, pstats++) >> + *pstats = 0; >> + } >> + >> + /* >> + * If a statistics count is in the middle of being updated, it >> + * is possible that the above clearing may not work. So we need >> + * to double check again to make sure that the counters are really >> + * cleared. Still there is a still a very small chance that the >> + * second clearing does not work. >> + */ >> + for_each_possible_cpu(cpu) { >> + unsigned long *pstats = per_cpu_ptr(pcs->stats, cpu); >> + int stat; >> + >> + for (stat = 0; stat< pcs->nstats; stat++, pstats++) >> + if (*pstats) >> + *pstats = 0; >> + } > I don't think this is acceptable. I am not sure what you mean here by not acceptable. Please enlighten me on that. >> +} >> + >> +static inline int percpu_stats_init(struct percpu_stats *pcs, int num) >> +{ >> + pcs->nstats = num; >> + pcs->stats = __alloc_percpu(sizeof(unsigned long) * num, >> + __alignof__(unsigned long)); >> + if (!pcs->stats) >> + return -ENOMEM; >> + >> + percpu_stats_reset(pcs); >> + return 0; >> +} >> + >> +static inline void percpu_stats_destroy(struct percpu_stats *pcs) >> +{ >> + free_percpu(pcs->stats); >> + pcs->stats = NULL; >> + pcs->nstats = 0; >> +} > Why inline the above functions? Will move this function to lib/percpu_stats.c. >> +static inline void >> +__percpu_stats_add(struct percpu_stats *pcs, int stat, int cnt) >> +{ >> + unsigned long *pstat; >> + >> + if ((unsigned int)stat>= pcs->nstats) >> + return; > This is a critical bug. Please don't fail silently. BUG_ON(), > please. Sure. > >> + preempt_disable(); >> + pstat = this_cpu_ptr(&pcs->stats[stat]); >> + *pstat += cnt; >> + preempt_enable(); >> +} > this_cpu_add() is atomic w.r.t. local operations. Will use this_cpu_add(). >> +static inline unsigned long >> +percpu_stats_sum(struct percpu_stats *pcs, int stat) >> +{ >> + int cpu; >> + unsigned long sum = 0; >> + >> + if ((unsigned int)stat>= pcs->nstats) >> + return sum; > Ditto. > >> + for_each_possible_cpu(cpu) >> + sum += per_cpu(pcs->stats[stat], cpu); >> + return sum; >> +} > Thanks. > Cheers, Longman