Messages in this thread | | | Date | Thu, 25 May 2006 16:03:08 +0800 | From | Wu Fengguang <> | Subject | Re: [PATCH 17/33] readahead: context based method |
| |
On Thu, May 25, 2006 at 03:26:00PM +1000, Nick Piggin wrote: > Wu Fengguang wrote: > >+ cond_resched(); > >+ read_lock_irq(&mapping->tree_lock); > >+ index = radix_tree_scan_hole_backward(&mapping->page_tree, > >+ offset, ra_max); > >+ read_unlock_irq(&mapping->tree_lock); > > > > Why do you drop this lock just to pick it up again a few instructions > down the line? (is ra_cache_hit_ok or cound_cache_hit very big or > unable to be called without the lock?)
Nice catch, will fix it.
> >+ > >+ *remain = offset - index; > >+ > >+ if (offset == ra->readahead_index && ra_cache_hit_ok(ra)) > >+ count = *remain; > >+ else if (count_cache_hit(mapping, index + 1, offset) * > >+ readahead_hit_rate >= > >*remain) > >+ count = *remain; > >+ else > >+ count = ra_min; > >+ > >+ /* > >+ * Unnecessary to count more? > >+ */ > >+ if (count < ra_max) > >+ goto out; > >+ > >+ if (unlikely(ra->flags & RA_FLAG_NO_LOOKAHEAD)) > >+ goto out; > >+ > >+ /* > >+ * Check the far pages coarsely. > >+ * The enlarged count here helps increase la_size. > >+ */ > >+ nr_lookback = ra_max * (LOOKAHEAD_RATIO + 1) * > >+ 100 / (readahead_ratio | 1); > >+ > >+ cond_resched(); > >+ radix_tree_cache_init(&cache); > >+ read_lock_irq(&mapping->tree_lock); > >+ for (count += ra_max; count < nr_lookback; count += ra_max) { > >+ struct radix_tree_node *node; > >+ node = radix_tree_cache_lookup_parent(&mapping->page_tree, > >+ &cache, offset - count, 1); > >+ if (!node) > >+ break; > >+ } > >+ read_unlock_irq(&mapping->tree_lock); > > > > Yuck. Apart from not being commented, this depends on internal > implementation of radix-tree. This should just be packaged up in some > radix-tree function to do exactly what you want (eg. is there a hole of > N contiguous pages).
Yes, it is ugly. Maybe we can make it a function named radix_tree_scan_hole_coarse().
> And then again you can be rid of the radix-tree cache. > > Yes, it increasingly appears that you're using the cache because you're > using the wrong abstractions. Eg. this is basically half implementing > some data-structure internal detail.
Sorry for not being aware of this problem :)
Wu - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |