lkml.org 
[lkml]   [2008]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCHv4] SGI UV: TLB shootdown using broadcast assist unit

* Cliff Wickman <cpw@sgi.com> wrote:

> +ENTRY(uv_bau_message_intr1)
> + apicinterrupt 220,uv_bau_message_interrupt
> +END(uv_bau_message_intr1)

style: we try to add a single space after commas, to make it visually
less crowded.

> @@ -162,6 +164,9 @@ void native_flush_tlb_others(const cpuma
> union smp_flush_state *f;
> cpumask_t cpumask = *cpumaskp;
>
> + if (is_uv_system() && uv_flush_tlb_others(&cpumask, mm, va))
> + return;

since you basically hook into this function and replace it, wouldnt it
make more sense instead to override the flush_tlb_others callback in
paravirt_ops? That way common code is not affected.

> +char *status_table[] = {
> + "IDLE",
> + "ACTIVE",
> + "DESTINATION TIMEOUT",
> + "SOURCE TIMEOUT"
> +};
> +
> +DEFINE_PER_CPU(struct ptc_stats, ptcstats);
> +DEFINE_PER_CPU(struct bau_control, bau_control);

shouldnt these be static too?

> + if (msp)
> + msp->seen_by.bits = 0;
> + uv_write_local_mmr(UVH_LB_BAU_INTD_SOFTWARE_ACKNOWLEDGE_ALIAS, dw);
> + return;
> +}

return is not needed at the end of void functions. (there are many other
similar incidents in the whole file)

> + this_cpu_mask = (unsigned long)1 << cpu;

why not 1UL?

> + sender = smp_processor_id();
> + for (i = 0; i < (sizeof(struct bau_target_nodemask) * BITSPERBYTE); i++) {
> + i++) {
> + if (!bau_node_isset(i, distribution))
> + continue;
> + bau_tablesp = uv_bau_table_bases[i];
> + for (msg = bau_tablesp->va_queue_first, j = 0;
> + j < DESTINATION_PAYLOAD_QUEUE_SIZE; msg++, j++) {
> + if ((msg->sending_cpu == sender) &&
> + (!msg->replied_to)) {
> + msp = bau_tablesp->msg_statuses + j;
> + printk(KERN_DEBUG
> + "blade %d: address:%#lx %d of %d, not cpu(s): ",
> + i, msg->address,
> + msg->acknowledge_count,
> + msg->number_of_cpus);
> + for (k = 0; k < msg->number_of_cpus;
> + k++) {
> + if (!((long)1 << k & msp->
> + seen_by.bits)) {
> + count++;
> + printk("%d ", k);
> + }
> + }
> + printk("\n");

why not split the iterator into a helper function? That would avoid all
these line length artifacts as well. Also, decrease constant name
length.

> +int uv_flush_send_and_wait(int cpu, int this_blade,
> + struct bau_activation_descriptor *bau_desc, cpumask_t *cpumaskp)

'struct bau_activation_descriptor' is too long of a name (and that
uglifies many prototypes) - why not use 'struct bau_desc'

> + time1 = get_cycles();
> + do {
> + tries++;
> + index = ((unsigned long)
> + 1 << UVH_LB_BAU_SB_ACTIVATION_CONTROL_PUSH_SHFT) | cpu;
> + uv_write_local_mmr(UVH_LB_BAU_SB_ACTIVATION_CONTROL, index);
> + completion_status = uv_wait_completion(bau_desc, mmr_offset,
> + right_shift);
> + } while (completion_status == FLUSH_RETRY);
> + time2 = get_cycles();

is get_cycles() guaranteed to be stable on UV platforms? No cpufreq?

> + pqp = __get_cpu_var(bau_control).va_queue_first;
> + msg = __get_cpu_var(bau_control).bau_msg_head;
> + while (msg->sw_ack_vector) {
> + count++;
> + fw = msg->sw_ack_vector;
> + msg_slot = msg - pqp;
> + sw_ack_slot = ffs(fw) - 1;
> +
> + uv_bau_process_message(msg, msg_slot, sw_ack_slot);
> +
> + msg++;
> + if (msg > __get_cpu_var(bau_control).va_queue_last)
> + msg = __get_cpu_var(bau_control).va_queue_first;
> + __get_cpu_var(bau_control).bau_msg_head = msg;

many repetitive __get_cpu_var(bau_control) instances - put that into a
helper variable instead, that makes it all much more readable.

> +static void uv_ptc_seq_stop(struct seq_file *file, void *data)
> +{
> +}

i think a NULL seq_stop will achieve a NOP. (not sure)

> +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
> +};

small nit, we try to keep vertical alignment consistent across files,
i.e.:

> +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
> +};

> + if (!proc_mkdir("sgi_uv", NULL))
> + return -EINVAL;
> +
> + proc_uv_ptc = create_proc_entry(UV_PTC_BASENAME, 0444, NULL);
> + if (!proc_uv_ptc) {
> + printk(KERN_ERR "unable to create %s proc entry\n",
> + UV_PTC_BASENAME);
> + return -EINVAL;
> + }

this failure branch will leak the sgi_uv directory.

> + bau_tablesp =
> + kmalloc_node(sizeof(struct bau_control), GFP_KERNEL, node);
> + if (!bau_tablesp)
> + BUG();

use BUG_ON(). (in other instances as well in this same file)

> + for (i = 0, ip = bau_tablesp->watching;
> + i < DESTINATION_PAYLOAD_QUEUE_SIZE; i++, ip++) {
> + *ip = 0;
> + }

this iteration could avoid the linebreak via intelligent shrinking of
overlong symbols:

> + for (i = 0, ip = bau_tablesp->watching; i < BAU_QUEUE_SIZE; i++, ip++)
> + *ip = 0;

(similar things in other places as well)

Ingo


\
 
 \ /
  Last update: 2008-06-12 15:15    [W:1.980 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site