On Wed, 22 Aug 2007 12:00:11 -0500 Dean Nelson <dcn@xxxxxxx> wrote: > > 3) WARNING: declaring multiple variables together should be avoided > > checkpatch.pl is erroneously commplaining about the following found in five > different functions in arch/ia64/sn/kernel/xpmem_pfn.c. > > int n_pgs = xpmem_num_of_pages(vaddr, size); What warning does it generate here? > > - xpmem_fault_handler() appears to have imposed a kernel-wide rule that > > when taking multiple mmap_sems, one should take the lowest-addressed one > > first? If so, that probably wants a mention in that locking comment in > > filemap.c > > Sure. After looking at the lock ordering comment block in mm/filemap.c, it > wasn't clear to me how best to document this. Any suggestions/help would > be most appreciated. umm, * when taking multiple mmap_sems, one should take the lowest-addressed one * first ;) > > - xpmem_fault_handler() does atomic_dec(&seg_tg->mm->mm_users). What > > happens if that was the last reference? > > When /dev/xpmem is opened by a user process, xpmem_open() incs mm_users > and when it is flushed, xpmem_flush() decs it (via mmput()) after having > ensured that no XPMEM attachments exist of this mm. Thus the dec in > xpmem_fault_handler() will never take it to 0. OK. Generally if a reviewer asks a question like this, it indicates that a code comment is needed. Because it is likely that others will later wonder the same thing. > > - Has it all been tested with lockdep enabled? Jugding from all the use > > of SPIN_LOCK_UNLOCKED, it has not. > > > > Oh, ia64 doesn't implement lockdep. For this code, that is deeply > > regrettable. > > No, it hasn't been tested with lockdep. But I have switched it from using > SPIN_LOCK_UNLOCKED to spin_lock_init(). > > > ! This code all predates the nopage->fault conversion and won't work in > > current kernels. > > I've switched from using nopage to using fault. I read that it is intended > that nopfn also goes away. If this is the case, then the BUG_ON if VM_PFNMAP > is set would make __do_fault() a rather unfriendly replacement for do_no_pfn(). > > > - xpmem_attach() does smp_processor_id() in preemptible code. Lucky that > > ia64 doesn't do preempt? > > Actually, the code is fine as is even with preemption configured on. All it's > doing is ensuring that the thread was previously pinned to the CPU it's > currently running on. If it is, it can't be moved to another CPU via > preemption, and if it isn't, the check will fail and we'll return -EINVAL > and all is well. OK. Running smp_processor_id() from within preemptible code will generate a warning, but the code is sneaky enough to prevent that warning if the calling task happens to be pinned to a single CPU. - To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
![]() |