Messages in this thread | | | Date | Tue, 5 Jun 2012 14:58:34 +0530 | From | Srikar Dronamraju <> | Subject | Re: [PATCH 0/3] uprobes: make register/unregister O(n) |
| |
> > 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
| |