Re: [PATCH] x86: UV uv_tlb.c cleanup
From: Ingo Molnar
Date: Tue May 17 2011 - 15:05:04 EST
Ok, looks like some good first steps. I have a few more nits below, of things
that occur still quite widely in the UV driver files:
* Cliff Wickman <cpw@xxxxxxx> wrote:
> +static void write_global_mmr_activation(int pnode, unsigned long mmr_image)
> +{
> + uv_write_global_mmr64(pnode, UVH_LB_BAU_SB_ACTIVATION_CONTROL,
> + mmr_image);
Please *never* break a line at the end line that just because checkpatch.pl
complains.
Instead try to make uv_write_global_mmr64() shorter, it's used in quite a few
places!
Something like:
write_gmmr8()
write_gmmr64()
write_lmmr64()
would still be pretty obvious and is a lot shorter! I applied two tricks:
- The 'MMR' is an UV specific term of art *anyway* which you have to know if
you are reading this code, so making it 'LMMR' for local and 'GMMR' for
global MMRs is an obvious extension to the abbreviation you are using already.
- You do not have to prefix everything with uv_() !! Only APIs you really need
to export to other code like the infamous is_uv_system() flag. These
functions are used in files named uv_something.c anyway, and 'MMR' is an UV
concept, so the uv_ prefix was absolutely unnecessary screen bloat.
Please propagate the uv_ removal to all other places - there's a lot of
functions that have it needlessly.
Likewise, you can propagate these concepts to the naming of the other MMR
handling functions as well - for example would turn into
write_gmmr_activation() - which is nicer to read and type than
write_global_mmr_activation().
I think with that and with your other cleanups you have already done the code
will already be a lot more readable.
One more request, could you please make structure fields aligned vertically?
I.e. use something like:
struct msg_desc {
struct bau_payload_queue_entry *msg;
int msg_slot;
int swack_slot;
struct bau_payload_queue_entry *va_queue_first;
struct bau_payload_queue_entry *va_queue_last;
};
Have a look at arch/x86/include/asm/processor.h for more examples.
Also, *please* notice and fix too long structure names like the
bau_payload_queue_entry above. Commonly used names like that should be tightly
named - so for example could it be 'struct bau_pq_entry'? If that's descriptive
enough for you then it could be:
struct msg_desc {
struct bau_pq_entry *msg;
int msg_slot;
int swack_slot;
struct bau_pq_entry *va_queue_first;
struct bau_pq_entry *va_queue_last;
};
Also, what does the 'va_' prefix mean above? Virtual address? Why does 'msg'
have no prefix?
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/