Re: [PATCH v2] x86: UV uv_tlb.c cleanup
From: Ingo Molnar
Date: Fri May 20 2011 - 09:31:37 EST
* Cliff Wickman <cpw@xxxxxxx> wrote:
> +static ssize_t tunables_write(struct file *file, const char __user *user,
> + size_t count, loff_t *data)
> +{
> + int cpu;
> + int ret;
> + char instr[100];
> + struct bau_control *bcp;
> +
> + if (count == 0 || count > sizeof(instr)-1)
> + return -EINVAL;
> + if (copy_from_user(instr, user, count))
> + return -EFAULT;
> + instr[count] = '\0';
> + bcp = &per_cpu(bau_control, smp_processor_id());
> + ret = parse_tunables_write(bcp, instr, count);
> + if (ret)
> + return ret;
> for_each_present_cpu(cpu) {
> bcp = &per_cpu(bau_control, cpu);
> - bcp->max_bau_concurrent = max_bau_concurrent;
> - bcp->max_bau_concurrent_constant = max_bau_concurrent;
> + bcp->max_concurr = max_concurr;
> + bcp->max_concurr_const = max_concurr;
> bcp->plugged_delay = plugged_delay;
> bcp->plugsb4reset = plugsb4reset;
> bcp->timeoutsb4reset = timeoutsb4reset;
> bcp->ipi_reset_limit = ipi_reset_limit;
> bcp->complete_threshold = complete_threshold;
> - bcp->congested_response_us = congested_response_us;
> - bcp->congested_reps = congested_reps;
> - bcp->congested_period = congested_period;
> + bcp->cong_response_us = congested_response_us;
> + bcp->cong_reps = congested_reps;
> + bcp->cong_period = congested_period;
> }
> return count;
> }
>
> static const struct seq_operations uv_ptc_seq_ops = {
> - .start = uv_ptc_seq_start,
> - .next = uv_ptc_seq_next,
> - .stop = uv_ptc_seq_stop,
> - .show = uv_ptc_seq_show
> + .start = ptc_seq_start,
> + .next = ptc_seq_next,
> + .stop = ptc_seq_stop,
> + .show = ptc_seq_show
Please apply vertical alignment for mass-initializations as well, like the ones further above.
This:
for_each_present_cpu(cpu) {
bcp = &per_cpu(bau_control, cpu);
bcp->max_concurr = max_concurr;
bcp->max_concurr_const = max_concurr;
bcp->plugged_delay = plugged_delay;
bcp->plugsb4reset = plugsb4reset;
bcp->timeoutsb4reset = timeoutsb4reset;
bcp->ipi_reset_limit = ipi_reset_limit;
bcp->complete_threshold = complete_threshold;
bcp->cong_response_us = congested_response_us;
bcp->cong_reps = congested_reps;
bcp->cong_period = congested_period;
}
Looks a *lot* tidier visually.
Another detail is the lack of separation between blocks of code:
> + if (count == 0 || count > sizeof(instr)-1)
> + return -EINVAL;
> + if (copy_from_user(instr, user, count))
> + return -EFAULT;
> + instr[count] = '\0';
> + bcp = &per_cpu(bau_control, smp_processor_id());
> + ret = parse_tunables_write(bcp, instr, count);
> + if (ret)
> + return ret;
> for_each_present_cpu(cpu) {
> bcp = &per_cpu(bau_control, cpu);
It looks more structured if it's written like this:
static ssize_t tunables_write(struct file *file, const char __user *user,
size_t count, loff_t *data)
{
int cpu;
int ret;
char instr[100];
struct bau_control *bcp;
if (count == 0 || count > sizeof(instr)-1)
return -EINVAL;
if (copy_from_user(instr, user, count))
return -EFAULT;
instr[count] = '\0';
bcp = &per_cpu(bau_control, smp_processor_id());
ret = parse_tunables_write(bcp, instr, count);
if (ret)
return ret;
for_each_present_cpu(cpu) {
bcp = &per_cpu(bau_control, cpu);
Let the code breath and keep bits together that belong together.
That way the reviewer can see the various key steps at a glance, the code
becomes more structured.
The patterns above repeat in many places in uv_tlb.c.
And clean code works: for example, when i look at this restructured code a real
bug in the code sticks out at me like a sore thumb: the smp_processor_id() is
called with preemption enabled, this will generate an ugly kernel warning when
this tunable is tweaked, with the right debug options turned on ...
Btw., please fix the bug in a separate patch, the cleanup patch itself should
have no functional changes at all.
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/