lkml.org 
[lkml]   [2012]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [patch] a simple hardware detector for latency as well as throughput ver. 0.1.0
Date
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@intel.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@vger.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


\
 
 \ /
  Last update: 2012-06-13 17:41    [W:0.098 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site