Message-Id: <201206131507.40470.arnd@arndb.de>
Date:	Wed, 13 Jun 2012 15:07:39 +0000
From:	Arnd Bergmann <arnd@...db.de>
To:	Luming Yu <luming.yu@...il.com>
Cc:	jcm@...masters.org, linux-kernel@...r.kernel.org
Subject: Re: [patch] a simple hardware detector for latency as well as throughput ver. 0.1.0

Hi Luming,

On Monday 28 May 2012, Luming Yu wrote:
> The patch is the fist step to test some basic hardware functions like
> TSC to help people understand if there is any hardware latency as well as
> throughput problem exposed on bare metal or left behind by BIOS or
> interfered by SM. Currently the patch tests hardware features
> (tsc,freq, and rdrandom whiich is new instruction to get random
> number) in stop_machine context. I will add more after the first step
> get merged for those guys who want to directly play with new hardware
> functions.
> 
> I suppose I can add your signed-off-by as the code is derived from your
> hwlat_dector.
> 
> I'm also reuqesting if you are going to queue it up somewhere that can
> be pulled into 3.5.
> 
> Of cause, I will update the patch based upon any comments that you
> think must be fixed for 3.5 merge.
> 
> Signed-off-by Luming Yu <luming.yu@...el.com>
> 
> 
>  Kconfig   |    7 
>  Makefile  |    2 
>  hw_test.c |  954 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 963 insertions(+)

Please write the text in the email so that it can be used as a permanent
changelog entry for the kernel git history. Everything that you do not
want to have in that history can go under the "---" line that separates
the Signed-off-by from the diffstat.

> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index c779509..f66ad56 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -123,6 +123,13 @@ config IBM_ASM
>  	  for information on the specific driver level and support statement
>  	  for your IBM server.
>  
> +config HW_TEST
> +	tristate "Testing module to detect hardware lattency and throughput"
> +	depends on DEBUG_FS
> +	depends on RING_BUFFER
> +	depends on X86
> +	default m
> +
>  config PHANTOM
>  	tristate "Sensable PHANToM (PCI)"
>  	depends on PCI

Any reason why this depends on X86?

I think the name "HW_TEST" is too generic. Maybe "HW_LATENCY_TEST"?

> +static struct ring_buffer *ring_buffer;
> +static DEFINE_MUTEX(ring_buffer_mutex);
> +static unsigned long buf_size = 262144UL;
> +static struct task_struct *kthread;
> +
> +struct sample;
> +struct data;
> +
> +struct sample_function {
> +	const char *name;
> +	struct list_head list;
> +	int (*get_sample)(void *unused);
> +};

why the "unused" argument?

> +static struct sample_function *current_sample_func = NULL;
> +static LIST_HEAD(sample_function_list);
> +static DEFINE_MUTEX(sample_function_mutex);
> +static int sample_function_register(struct sample_function *sf);
> +
> +static struct dentry *debug_dir;
> +static struct dentry *debug_available;
> +static struct dentry *debug_current;
> +static struct dentry *debug_max;
> +static struct dentry *debug_count;
> +static struct dentry *debug_sample_width;
> +static struct dentry *debug_sample_window;
> +static struct dentry *debug_sample;
> +static struct dentry *debug_threshold;
> +static struct dentry *debug_enable;

I think it would help here to put the local variables into a data structure
that gets allocated at module load time.

> +static int __buffer_add_sample(struct sample *sample);
> +static struct sample *buffer_get_sample(struct sample *sample);
> +
...
> +static ssize_t debug_enable_fwrite(struct file *filp, 
> +				const char __user *user_buffer,
> +				size_t user_size, loff_t *offset);
> +
> +static int init_debugfs(void);
> +static void free_debugfs(void);
> +static int hw_test_init(void);
> +static void hw_test_exit(void);

Please reorder all the functions so that you no longer need forward
declarations.

> +static int get_random_bytes_sample(void *unused)
> +{
> +	u32 *buffer;
> +	ktime_t	start, t1, t2;
> +	s64	diff, total = 0;
> +	u64	sample = 0;
> +	int	ret = 1;
> +
> +	buffer = kzalloc(1024, GFP_KERNEL);
> +
> +	start = ktime_get();
> +	do {
> +
> +		t1 = ktime_get();
> +		get_random_bytes(buffer, 1024);
> +		t2 = ktime_get();
> +		total = ktime_to_us(ktime_sub(t2, start));
> +		diff = ktime_to_us(ktime_sub(t2, t1));

You need more comments to explain why you are doing things. For instance
here: why is it a useful thing to test how long get_random_bytes()
takes?

> +static int get_freq_sample(void *unused)
> +{
> +	ktime_t	start, t1, t2;
> +	s64	diff, total = 0;
> +	u32	sample = 0;
> +	int	ret = 1;
> +	unsigned int cpu_tsc_freq;
> +	static DEFINE_MUTEX(freq_pit_mutex);
> +
> +	start = ktime_get();
> +	do {
> +		t1 = ktime_get();
> +		mutex_lock(&freq_pit_mutex);
> +		cpu_tsc_freq = x86_platform.calibrate_tsc();
> +		mutex_unlock(&freq_pit_mutex);

Or the calibrate_tsc() function.

It's also not clear what function the freq_pit_mutex has. I don't
see how the code could ever get run twice at the same time.

> +static int kthread_fn(void *unused)
> +{
> +	int err = 0;
> +	u64 interval = 0;
> +	int (*get_sample)(void *unused);
> +	
> +	mutex_lock(&sample_function_mutex);
> +	if (current_sample_func)
> +static int hw_test_init(void)
> +{
> +	int ret = -ENOMEM;
> +	
> +	printk(KERN_INFO BANNER "version %s\n", VERSION);
> +
> +	sample_function_register(&tsc_sample);
> +	sample_function_register(&tsc_freq_sample);
> +	sample_function_register(&random_bytes_sample);
> +
> +	ret = init_stats();
> +	if (0 != ret)
> +		goto out;
> +	ret = init_debugfs();
> +	if (0 != ret)
> +		goto err_stats;
> +	if (enabled)
> +		ret = start_kthread();
> +	goto out;
> +
> +err_stats:
> +	ring_buffer_free(ring_buffer);
> +out:
> +	return ret;
> +}
> +
> +static void hw_test_exit(void)
> +{
> +	int err;
> +	
> +	if (enabled) {
> +		enabled = 0;
> +		err = stop_kthread();
> +		if (err)
> +			printk(KERN_ERR BANNER "cannot stop kthread\n");
> +	}
> +	
> +	free_debugfs();
> +	ring_buffer_free(ring_buffer);
> +}
> +
> +module_init(hw_test_init);
> +module_exit(hw_test_exit);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

> +		get_sample = current_sample_func->get_sample;
> +	else
> +		goto out;
> +
> +	while (!kthread_should_stop()) {
> +		mutex_lock(&data.lock);
> +	
> +		err = stop_machine(get_sample, unused, cpu_online_mask);
> +		if (err) {
> +			mutex_unlock(&data.lock);
> +			goto err_out;
> +		}
> +
> +		wake_up(&data.wq);

Using stop_machine() sounds like a very expensive thing to do here.
Is it necessary?

> +		interval = data.sample_window - data.sample_width;
> +		do_div(interval, USEC_PER_MSEC);

Have you tried avoiding the need for do_div? You could use an "unsigned long"
to count the microseconds here, or you could do everything using miliseconds.

> +static int start_kthread(void)
> +{
> +	kthread = kthread_run(kthread_fn, NULL, DRVNAME);
> +	if (IS_ERR(kthread)) {
> +		printk(KERN_ERR BANNER "could not start sampling thread\n");
> +		enabled = 0;
> +		return -ENOMEM;
> +	}
> +	return 0;
> +}
> +
> +static int stop_kthread(void)
> +{
> +	int ret;
> +	ret = kthread_stop(kthread);
> +	return ret;
> +}

These helper functions do not appear to help. Just remove them.

> +static ssize_t simple_data_read(struct file *filp, char __user *ubuf,
> +				size_t cnt, loff_t *ppos, const u64 *entry)
> +{
> +	char buf[U64STR_SIZE];
> +	u64 val = 0;
> +	int len = 0;
> +
> +	memset(buf, 0, sizeof(buf));
> +	if (!entry)
> +		return -EFAULT;
> +	mutex_lock(&data.lock);
> +	val = *entry;
> +	mutex_unlock(&data.lock);
> +	len = snprintf(buf, sizeof(buf), "%llu\n", (unsigned long long)val);
> +	return simple_read_from_buffer(ubuf, cnt, ppos, buf, len);
> +}
> +
> +static ssize_t simple_data_write(struct file *filp, const char __user *ubuf,
> +				size_t cnt, loff_t *ppos, u64 *entry)
> +{
> +	char buf[U64STR_SIZE];
> +	int csize = min(cnt, sizeof(buf));
> +	u64 val = 0;
> +	int err = 0;
> +
> +	memset(buf, '\0', sizeof(buf));
> +	if (copy_from_user(buf, ubuf, csize))
> +		return -EFAULT;
> +	buf[U64STR_SIZE-1] = '\0';
> +	err = strict_strtoull(buf, 10, &val);
> +	if (err)
> +		return -EINVAL;
> +	mutex_lock(&data.lock);
> +	*entry = val;
> +	mutex_unlock(&data.lock);
> +	return csize;
> +}

These look like you can use DEFINE_SIMPLE_ATTRIBUTE() or debugfs_create_u64()
to simplify your code.

> +static int debug_available_fopen(struct inode *inode, struct file *filp)
> +{
> +	return 0;
> +}

Just use simple_open() or no open function in the file_operations, there is
no need to provide an empty function.

> +static ssize_t debug_available_fwrite(struct file *filp, const char __user *ubuf,
> +					size_t	cnt, loff_t *ppos)
> +{
> +	return (ssize_t) 0;
> +}

Same for empty write functions.

> +static int init_debugfs(void)
> +{
> +	int ret = -ENOMEM;
> +
> +	debug_dir = debugfs_create_dir(DRVNAME, NULL);
> +	if (!debug_dir)
> +		goto err_debug_dir;
> +
> +	debug_available = debugfs_create_file("available", 0444,
> +						debug_dir, NULL,
> +						&available_fops);
> +	if (!debug_available)
> +		goto err_available;
> +
> +	debug_current = debugfs_create_file("current", 0444,
> +						debug_dir, NULL,
> +						&current_fops);
> +	if (!debug_current)
> +		goto err_current;
> +
> +	debug_sample = debugfs_create_file("sample", 0444,
> +						debug_dir, NULL,
> +						&sample_fops);
> +	if (!debug_sample)
> +		goto err_sample;
> +	
> +	debug_count = debugfs_create_file("count", 0444,
> +						debug_dir, NULL,
> +						&count_fops);

Just use an array of file_operations and create the files using a loop.


	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists