Re: [PATCH 0/3] uprobes: make register/unregister O(n)

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

> > This is more theory
> > If the number of vmas in the priority tree keeps increasing in every
> > iteration, and the kmalloc(GFP_NOWAIT) fails i.e more is !0, then
> > dont we end up in a forever loop?
> 
> But this can only hapen if the number of mappings keeps increasing
> indefinitely, this is not possible.

Agree, that why I said in theory.

> 
> And please not that the current code is worse if the number of mappings
> grows. In fact it can livelock even if this number is limited. Suppose
> you are trying to probe /bin/true and some process does
> "while true; do /bin/true; done". It is possible that every time
> register_for_each_vma() finds the new mapping because we do not free
> the previous entries which refer to the already dead process/mm.
> 

Completely agree that build_map_info is much better the current implementation. 
I was only trying understand and if possible simplify.

> 
> > Cant we just restrict this to just 2 iterations? [And depend on
> > uprobe_mmap() to do the necessary if new vmas come in].
> 

> Probably yes, we can improve this. But I don't think this is that
> easy, we can't rely on vma_prio_tree_foreach's ordering.
> 

Correct, I forgot that we cant rely on vma_prio_tree_for_each ordering.

> > >  out:
> >
> > > 	while (prev)
> > > 		prev = free_map_info(prev);
> >
> > If we were able to allocate all map_info objects in the first pass but
> > the last vma belonged to a mm thats at exit, i.e atomic_inc_non_zero
> > returned 0 , then prev is !NULL and more is 0.  Then we seem to clear
> > all the map_info objects without even decreasing the mm counts for which
> > atomic_inc_non_zero() was successful. Will curr be proper in this case.


I missed the point that prev->next is NULL when atomic_inc_non_zero
fails.

> 
> Not sure I understand. To simplify, suppose that we have a single mapping
> but atomic_inc_non_zero() fails. In this case, yes, prev != NULL but
> prev->mm is not valid. We only need to free the "extra" memory and return
> curr == NULL (empty list). This is what the loop above does.
> 
> We only need mmput(mm) if atomic_inc_non_zero() suceeeds, and in this
> case this "mm" should live in "curr" list. If we need more memory, then
> build_map_info() does this right after it detects more != 0. Otherwise
> the caller should do this.
> 

Correct. I understand now.

> > Should this while be an if?
> 
> But we can have more entries to free? Not only because atomic_inc_non_zero
> failed.
> 

Oh Yes, we could have entries to free because the vmas in the prio-tree
might have decreased by the time we walk the prio-tree the second time.

-- 
Thanks and Regards
Srikar