[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-arm-kernel
Subject:    Re: [PATCH] Section based-ioremap attempt #2
From:       Deepak Saxena <dsaxena () plexity ! net>
Date:       2005-09-29 19:26:40
Message-ID: 20050929192640.GA28911 () plexity ! net
[Download RAW message or body]

On Sep 29 2005, at 00:17, Nicolas Pitre was caught saying:
> +static int
> +remap_area_sections(unsigned long addr, unsigned long phys_addr, 
> +			unsigned long size, unsigned long flags)
> +{
> +	int prot = PMD_TYPE_SECT | PMD_DOMAIN(DOMAIN_IO) | PMD_SECT_AP_WRITE |\
> +			(flags & (L_PTE_CACHEABLE | L_PTE_BUFFERABLE)) |\
> +			((flags & L_PTE_USER) ? PMD_SECT_AP_READ : 0);
> +	pgd_t *dir = pgd_offset(&init_mm, addr);
> +	pmd_t *pmd = pmd_alloc(&init_mm, dir, addr);
> +
> +	if (!pmd)
> +		return -ENOMEM;
> 
> There is no point in calling pmd_alloc() since there is no extra memory 
> required for a first level mapping.  See alloc_init_section() for 
> example (and note the kirk with the virtual address since the pmd 
> representation is usually 2MB aligned).

Good point.

> +
> +	init_mm.context.kvm_seq++;
> 
> NO.  Here you're completely bypassing the lazy update and actually 
> forcing a full page table update for the vm area for every process 
> that'll be scheduled after every ioremap.  That's bad.  The sequence 
> number should really be increased only in the iounmap case and in the 
> "section mapping replaces a second level mapping" case above.  
> Everything else should work out with the lazy update automatically 
> with no sequence number increase.

Agreed.

> +	/*
> +	 * If this is a section based mapping we need to handle it 
> +	 * specially as the VM subysystem does not know how to handle
> +	 * such a beast.
> +	 */
> +	write_lock(&vmlist_lock);
> +	for (p = &vmlist ; (tmp = *p) ; p = &tmp->next) {
> +		if((tmp->flags & VM_IOREMAP) && (tmp->addr == addr)) {
> +			if (tmp->flags & VM_ARM_SECTION_MAPPING) {
> +
> +				*p = tmp->next;
> +
> +				unmap_vm_area(tmp);
> 
> I don't think unmap_vm_area will do the job appropriately.  It expects 
> the regular multi-level page tables and section mapping entries might 
> only confuse it.  I think the section clearing should probably be done 
> by hand and to both &init_mm and current->mm for the same reason 
> mentioned previously.  This should also get rid of the hack in trap.c.

unmap_vm_area() will do the job with the hack, but I would rather not 
have that hack and walking the tables is easy enough to do.

> Again this should be increased only if section mappings were removed 
> i.e. only if found is true.

Yep. 

Tnx,
~Deepak


-- 
Deepak Saxena - dsaxena@plexity.net - http://www.plexity.net

Even a stopped clock gives the right time twice a day.

-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php
[prev in list] [next in list] [prev in thread] [next in thread]