Jes Sorensen
2008-Mar-31 09:03 UTC
[01/17]PATCH Add API for allocating dynamic TR resouce. V8
Hi Xiantao, I general I think the code in this patch is fine. I have a couple of nit-picking comments:> + if (target_mask&0x1) {The formatting here isn't quite what most of the kernel does. It would be better if you added spaces so it's a little easier to read, ie: if (target_mask & 0x1) {> + p = &__per_cpu_idtrs[cpu][0][0]; > + for (i = IA64_TR_ALLOC_BASE; i <= per_cpu(ia64_tr_used, > cpu); > + i++, > p++) { > + if (p->pte&0x1)Same thing here.> +#define RR_TO_RID(rr) ((rr)<<32>>40)I would prefer to have this one defined like this: #define RR_TO_RID(rr) (rr >> 8) & 0xffffff It should generate the same code, but is more intuitive for the reader. Otherwise I think this patch is fine - this is really just cosmetics. Cheers, Jes
Carsten Otte
2008-Mar-31 13:41 UTC
[01/17]PATCH Add API for allocating dynamic TR resouce. V8
Zhang, Xiantao wrote:> +/* mca_insert_tr > + * > + * Switch rid when TR reload and needed! > + * iord: 1: itr, 2: itr; > + * > +*/ > +static void mca_insert_tr(u64 iord) > +{ > + > + int i; > + u64 old_rr; > + struct ia64_tr_entry *p; > + unsigned long psr; > + int cpu = smp_processor_id();What if CONFIG_PREEMPT is on, and we're being preempted and scheduled to a different CPU here? Are we running preempt disabled here? If so, the function header should state that this function needs to be called preempt_disabled.> +/* > + * ia64_insert_tr in virtual mode. Allocate a TR slot > + * > + * target_mask : 0x1 : itr, 0x2 : dtr, 0x3 : idtr > + * > + * va : virtual address. > + * pte : pte entries inserted. > + * log_size: range to be covered. > + * > + * Return value: <0 : error No. > + * > + * >=0 : slot number allocated for TR. > + */ > +int ia64_itr_entry(u64 target_mask, u64 va, u64 pte, u64 log_size) > +{ > + int i, r; > + unsigned long psr; > + struct ia64_tr_entry *p; > + int cpu = smp_processor_id();Same here.> +/* > + * ia64_purge_tr > + * > + * target_mask: 0x1: purge itr, 0x2 : purge dtr, 0x3 purge idtr. > + * > + * slot: slot number to be freed. > + */ > +void ia64_ptr_entry(u64 target_mask, int slot) > +{ > + int cpu = smp_processor_id(); > + int i; > + struct ia64_tr_entry *p;Here again.
Carsten Otte
2008-Apr-01 07:48 UTC
[01/17]PATCH Add API for allocating dynamic TR resouce. V8
Zhang, Xiantao wrote:> Carsten Otte wrote: >> Zhang, Xiantao wrote: >>> +/* mca_insert_tr >>> + * >>> + * Switch rid when TR reload and needed! >>> + * iord: 1: itr, 2: itr; >>> + * >>> +*/ >>> +static void mca_insert_tr(u64 iord) >>> +{ >>> + >>> + int i; >>> + u64 old_rr; >>> + struct ia64_tr_entry *p; >>> + unsigned long psr; >>> + int cpu = smp_processor_id(); >> What if CONFIG_PREEMPT is on, and we're being preempted and scheduled >> to a different CPU here? Are we running preempt disabled here? If so, >> the function header should state that this function needs to be called >> preempt_disabled. > > The function insert one TR to local TLB, and doesn't allow preempt > before and after the call, so the caller should be with preempt_disable > before calling into this routine. > Maybe the descripiton of this function should contain "Called with > preempt disabled!". Does it make sense ?Yea, I think a comment would help in that case :-).