Re: [PATCH] abstract out bits of ldt.c

[Dave Hansen]

On Sun, 2005-08-07 at 16:20 -0700, Martin J. Bligh wrote: 
> Starting on the work to merge xen cleanly as a subarch.
> Introduce make_pages_readonly and make_pages_writable where appropriate 
> for Xen, defined as a no-op on other subarches. Same for 
> add_context_to_unpinned and del_context_from_unpinned.
> Abstract out install_ldt_entry()
... 
>  		cpumask_t mask;
>  		preempt_disable();
> +		make_pages_readonly(pc->ldt, (pc->size * LDT_ENTRY_SIZE) / 
> +								PAGE_SIZE);
>  		load_LDT(pc);
>  		mask = cpumask_of_cpu(smp_processor_id());
>  		if (!cpus_equal(current->mm->cpu_vm_mask, mask))
>  			smp_call_function(flush_ldt, NULL, 1, 1);
>  		preempt_enable();
>  #else
> +		make_pages_readonly(pc->ldt, (pc->size * LDT_ENTRY_SIZE) / 
> +								PAGE_SIZE);

You do that (size * LDT_ENTRY_SIZE / PAGE_SIZE) operation an awfully
large number of times.  Could you consider introducing a little helper,
say ldt_size_pages()?  

Or, could you have a helper like make_ldt_readonly()?  You don't have to
export it, just use it in that one file.

> This will do have no effect whatsover on platforms other than xen.
...
> +       memset(&mm->context, 0, sizeof(mm->context));
>         init_MUTEX(&mm->context.sem);
> -       mm->context.size = 0;

Could you please explain what this is for?  It doesn't appear to be part
of the abstraction.  

Every call path I can see to init_new_context() is immediately preceded
by mm_alloc(), which memsets the entire mm.  The context is a direct
member of mm_struct, and should be zeroed along with the mm_alloc()
memset.  So, this seems a bit superfluous.

In any future patches that you might post, please do one thing per
patch, it makes them much easier to audit.

-- Dave

-
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/