Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1503346

Re: [PATCH 6/6] mm: add preempt points into __purge_vmap_area_lazy

Show all headers | View raw

On Tue, Oct 18, 2016 at 08:56:11AM +0200, Christoph Hellwig wrote:
> From: Joel Fernandes <joelaf@google.com>
> 
> Use cond_resched_lock to avoid holding the vmap_area_lock for a
> potentially long time.
> 
> Signed-off-by: Joel Fernandes <joelaf@google.com>
> [hch: split from a larger patch by Joel, wrote the crappy changelog]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  mm/vmalloc.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 6c7eb8d..98b19ea 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -628,7 +628,7 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end)
>  	struct llist_node *valist;
>  	struct vmap_area *va;
>  	struct vmap_area *n_va;
> -	int nr = 0;
> +	bool do_free = false;
>  
>  	lockdep_assert_held(&vmap_purge_lock);
>  
> @@ -638,18 +638,22 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end)
>  			start = va->va_start;
>  		if (va->va_end > end)
>  			end = va->va_end;
> -		nr += (va->va_end - va->va_start) >> PAGE_SHIFT;
> +		do_free = true;
>  	}
>  
> -	if (!nr)
> +	if (!do_free)
>  		return false;
>  
> -	atomic_sub(nr, &vmap_lazy_nr);
>  	flush_tlb_kernel_range(start, end);
>  
>  	spin_lock(&vmap_area_lock);
> -	llist_for_each_entry_safe(va, n_va, valist, purge_list)
> +	llist_for_each_entry_safe(va, n_va, valist, purge_list) {
> +		int nr = (va->va_end - va->va_start) >> PAGE_SHIFT;
> +
>  		__free_vmap_area(va);
> +		atomic_sub(nr, &vmap_lazy_nr);
> +		cond_resched_lock(&vmap_area_lock);

Is releasing the lock within a llist_for_each_entry_safe() actually safe? Is
vmap_area_lock the one to protect the valist?

That is llist_for_each_entry_safe(va, n_va, valist, purg_list) does:

	for (va = llist_entry(valist, typeof(*va), purge_list);
	     &va->purge_list != NULL &&
	     n_va = llist_entry(va->purge_list.next, typeof(*n_va),
				purge_list, true);
	     pos = n)

Thus n_va is pointing to the next element to process when we release the
lock. Is it possible for another task to get into this same path and process
the item that n_va is pointing to? Then when the preempted task comes back,
grabs the vmap_area_lock, and then continues the loop with what n_va has,
could that cause problems? That is, the next iteration after releasing the
lock does va = n_va. What happens if n_va no longer exits?

I don't know this code that well, and perhaps vmap_area_lock is not protecting
the list and this is all fine.

-- Steve


> +	}
>  	spin_unlock(&vmap_area_lock);
>  	return true;
>  }
> -- 
> 2.1.4

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[RFC] reduce latency in __purge_vmap_area_lazy Christoph Hellwig <hch@lst.de> - 2016-10-18 09:00 +0200
  [PATCH 5/6] mm: turn vmap_purge_lock into a mutex Christoph Hellwig <hch@lst.de> - 2016-10-18 09:00 +0200
  [PATCH 2/6] mm: mark all calls into the vmalloc subsystem as potentially sleeping Christoph Hellwig <hch@lst.de> - 2016-10-18 09:00 +0200
    Re: [PATCH 2/6] mm: mark all calls into the vmalloc subsystem as  potentially sleeping Chris Wilson <chris@chris-wilson.co.uk> - 2016-10-18 12:40 +0200
      Re: [PATCH 2/6] mm: mark all calls into the vmalloc subsystem as         potentially sleeping Christoph Hellwig <hch@lst.de> - 2016-10-18 12:40 +0200
    Re: [PATCH 2/6] mm: mark all calls into the vmalloc subsystem as         potentially sleeping Christoph Hellwig <hch@lst.de> - 2016-10-19 16:30 +0200
      Re: [PATCH 2/6] mm: mark all calls into the vmalloc subsystem as  potentially sleeping Andy Lutomirski <luto@amacapital.net> - 2016-10-19 17:40 +0200
        Re: [PATCH 2/6] mm: mark all calls into the vmalloc subsystem as         potentially sleeping Christoph Hellwig <hch@lst.de> - 2016-10-19 18:40 +0200
          Re: [PATCH 2/6] mm: mark all calls into the vmalloc subsystem as  potentially sleeping Chris Wilson <chris@chris-wilson.co.uk> - 2016-10-19 21:50 +0200
          Re: [PATCH 2/6] mm: mark all calls into the vmalloc subsystem as  potentially sleeping Joel Fernandes <joelaf@google.com> - 2016-10-21 02:40 +0200
    Re: [PATCH 2/6] mm: mark all calls into the vmalloc subsystem as  potentially sleeping Chris Wilson <chris@chris-wilson.co.uk> - 2016-10-19 16:40 +0200
  [PATCH 1/6] mm: refactor __purge_vmap_area_lazy Christoph Hellwig <hch@lst.de> - 2016-10-18 09:00 +0200
  [PATCH 4/6] mm: remove free_unmap_vmap_area_addr Christoph Hellwig <hch@lst.de> - 2016-10-18 09:00 +0200
    Re: [PATCH 4/6] mm: remove free_unmap_vmap_area_addr Joel Fernandes <joelaf@google.com> - 2016-10-21 02:50 +0200
      Re: [PATCH 4/6] mm: remove free_unmap_vmap_area_addr Nicholas Piggin <npiggin@gmail.com> - 2016-10-21 04:00 +0200
  [PATCH 3/6] mm: remove free_unmap_vmap_area_noflush Christoph Hellwig <hch@lst.de> - 2016-10-18 09:00 +0200
  [PATCH 6/6] mm: add preempt points into __purge_vmap_area_lazy Christoph Hellwig <hch@lst.de> - 2016-10-18 09:00 +0200
    Re: [PATCH 6/6] mm: add preempt points into __purge_vmap_area_lazy Steven Rostedt <rostedt@goodmis.org> - 2016-10-18 23:00 +0200
      Re: [PATCH 6/6] mm: add preempt points into __purge_vmap_area_lazy Steven Rostedt <rostedt@goodmis.org> - 2016-10-18 23:10 +0200
  Re: [RFC] reduce latency in __purge_vmap_area_lazy Nicholas Piggin <npiggin@gmail.com> - 2016-10-18 12:50 +0200
  Re: [RFC] reduce latency in __purge_vmap_area_lazy Jisheng Zhang <jszhang@marvell.com> - 2016-10-18 13:30 +0200
  Re: [RFC] reduce latency in __purge_vmap_area_lazy Joel Fernandes <joelaf@google.com> - 2016-10-21 03:10 +0200

csiph-web