From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751367AbdAPQHY (ORCPT ); Mon, 16 Jan 2017 11:07:24 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:41294 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750924AbdAPQHV (ORCPT ); Mon, 16 Jan 2017 11:07:21 -0500 Date: Mon, 16 Jan 2017 17:07:17 +0100 (CET) From: Thomas Gleixner To: Vikas Shivappa cc: vikas.shivappa@intel.com, linux-kernel@vger.kernel.org, x86@kernel.org, hpa@zytor.com, mingo@kernel.org, peterz@infradead.org, ravi.v.shankar@intel.com, tony.luck@intel.com, fenghua.yu@intel.com, h.peter.anvin@intel.com Subject: Re: [PATCH 7/8] x86/intel_rdt/mba: Add schemata file support for MBA In-Reply-To: <1484076788-25385-8-git-send-email-vikas.shivappa@linux.intel.com> Message-ID: References: <1484076788-25385-1-git-send-email-vikas.shivappa@linux.intel.com> <1484076788-25385-8-git-send-email-vikas.shivappa@linux.intel.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 10 Jan 2017, Vikas Shivappa wrote: > + * @display_str: Format string to show schemata > + * @validate: API to validate the ctrl values. > * @info_files: resctrl info files for the resource > * @infofiles_len: Number of info files > * @max_delay: Max throttle delay > @@ -99,6 +101,9 @@ struct rdt_resource { > int cbm_len; > int min_cbm_bits; > u32 no_ctrl; > + char *display_str; > + int (*validate) (char *buf, unsigned long *data, > + struct rdt_resource *r); Again this display and validation change wants to be seperate from the bandwidth stuff. It's not rocket science to split patches into preparatory and implementation parts. > + r->display_str = kstrdup("%d=%d", GFP_KERNEL); > + if (!r->display_str) > + return -ENOMEM; And the point of this allocation is? To consume extra memory for a constant string which is in const data anyway. r->display_str = "%d=%d"; does not need allcotion and consumes exactly the same amount of const data as the above. Oh well... > -static inline bool get_rdt_resources(void) > +static inline int get_rdt_resources(void) > { And the point of this change is? Lots of churn to return the same -ENODEV value at the call site. So why are you trying to return other values instead of the simple boolean success/fail decision? > /* > + * Check whether MBE 'throttle by' value is correct. > + * As per the SDM, when the scale is linear the > + * throttle_by granularity is '100 - max_thrtl_by' > + * and when its non-linear it is 'power of 2'. That's wrong. We really want to let the user set a bandwidth percentage value from 0 - 100 %. And then adjust it to the proper value which the hardware can provide. So the user value is independent from granularity, linear and the max throttling allowed. > /* > - * Read one cache bit mask (hex). Check that it is valid for the current > - * resource type. > + * Read the user RDT control value into tempory buffer: > + * Cache bit mask (hex) or Memory b/w throttle (decimal). > + * Check that it is valid for the current resource type. > */ > -static int parse_cbm(char *buf, struct rdt_resource *r) > +static int parse_ctrls(char *buf, struct rdt_resource *r) > { > unsigned long data; > - int ret; > + int ret = 0; What's the purpose of initializing ret to 0 if the next action is assigning ret the return value of the validate function? > - ret = kstrtoul(buf, 16, &data); > - if (ret) > - return ret; > - if (!cbm_validate(data, r)) > - return -EINVAL; > + ret = r->validate(buf, &data, r); > r->tmp_ctrl[r->num_tmp_ctrl++] = data; > > - return 0; > + return ret; > } Thanks, tglx