linux-kernel - Re: [PATCH 2/2] scsi: ufs-qcom: Add one sysfs node to monitor performance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 06 Jan 2021 08:51:42 +0800
From:   Can Guo <cang@...eaurora.org>
To:     Greg KH <greg@...ah.com>
Cc:     asutoshd@...eaurora.org, nguyenb@...eaurora.org,
        hongwus@...eaurora.org, rnayak@...eaurora.org,
        linux-scsi@...r.kernel.org, kernel-team@...roid.com,
        saravanak@...gle.com, salyzyn@...gle.com,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Avri Altman <avri.altman@....com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] scsi: ufs-qcom: Add one sysfs node to monitor
 performance

On 2021-01-05 18:27, Greg KH wrote:
> Oops, forgot the big problem that I noticed:
> 
> On Mon, Jan 04, 2021 at 07:15:51PM -0800, Can Guo wrote:
>> +static ssize_t monitor_show(struct device *dev, struct 
>> device_attribute *attr,
>> +			    char *buf)
>> +{
>> +	struct ufs_hba *hba = dev_get_drvdata(dev);
>> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> +	struct ufs_qcom_perf_monitor *mon = &host->monitor;
>> +	unsigned long nr_sec_rd, nr_sec_wr, busy_us_rd, busy_us_wr;
>> +	unsigned long lat_max_rd, lat_min_rd, lat_sum_rd, lat_avg_rd, 
>> nr_req_rd;
>> +	unsigned long lat_max_wr, lat_min_wr, lat_sum_wr, lat_avg_wr, 
>> nr_req_wr;
>> +	bool is_enabled;
>> +
>> +	/*
>> +	 * Don't lock the host lock since user needs to cat the entry very
>> +	 * frequently during performance test, otherwise it may impact the
>> +	 * performance.
>> +	 */
>> +	is_enabled = mon->enabled;
>> +	if (!is_enabled)
>> +		goto print_usage;
>> +
>> +	nr_sec_rd = mon->nr_sec_rw[READ];
>> +	nr_sec_wr = mon->nr_sec_rw[WRITE];
>> +	busy_us_rd = ktime_to_us(mon->total_busy[READ]);
>> +	busy_us_wr = ktime_to_us(mon->total_busy[WRITE]);
>> +
>> +	nr_req_rd = mon->nr_req[READ];
>> +	lat_max_rd = ktime_to_us(mon->lat_max[READ]);
>> +	lat_min_rd = ktime_to_us(mon->lat_min[READ]);
>> +	lat_sum_rd = ktime_to_us(mon->lat_sum[READ]);
>> +	lat_avg_rd = lat_sum_rd / nr_req_rd;
>> +
>> +	nr_req_wr = mon->nr_req[WRITE];
>> +	lat_max_wr = ktime_to_us(mon->lat_max[WRITE]);
>> +	lat_min_wr = ktime_to_us(mon->lat_min[WRITE]);
>> +	lat_sum_wr = ktime_to_us(mon->lat_sum[WRITE]);
>> +	lat_avg_wr = lat_sum_wr / nr_req_wr;
>> +
>> +	return scnprintf(buf, PAGE_SIZE, "Read %lu %s %lu us, %lu %s max %lu 
>> | min %lu | avg %lu | sum %lu\nWrite %lu %s %lu us, %lu %s max %lu | 
>> min %lu | avg %lu | sum %lu\n",
>> +		 nr_sec_rd, "sectors (in 512 bytes) in ", busy_us_rd,
>> +		 nr_req_rd, "read reqs completed, latencies in us: ",
>> +		 lat_max_rd, lat_min_rd, lat_avg_rd, lat_sum_rd,
>> +		 nr_sec_wr, "sectors (in 512 bytes) in ", busy_us_wr,
>> +		 nr_req_wr, "write reqs completed, latencies in us: ",
>> +		 lat_max_wr, lat_min_wr, lat_avg_wr, lat_sum_wr);
> 
> sysfs is one-value-per-file, not
> throw-everything-in-one-file-and-hope-userspace-can-parse-it.
> 
> This is not acceptable at all.  Why not just use debugfs for stats like
> this?
> 
> Also, use sysfs_emit() for any new sysfs files please.
> 
> thanks,
> 
> greg k-h

Hi Greg,

Thanks for the comments, I will rework the change to make it right.

Regards,
Can Guo.