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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |