Messages in this thread |  | | Date | Thu, 25 May 2006 15:26:00 +1000 | | From | Nick Piggin <> | | Subject | Re: [PATCH 17/33] readahead: context based method |
| |
Wu Fengguang wrote:
> >+/* >+ * Look back and check history pages to estimate thrashing-threshold. >+ */ >+static unsigned long query_page_cache_segment(struct address_space *mapping, >+ struct file_ra_state *ra, >+ unsigned long *remain, pgoff_t offset, >+ unsigned long ra_min, unsigned long ra_max) >+{ >+ pgoff_t index; >+ unsigned long count; >+ unsigned long nr_lookback; >+ struct radix_tree_cache cache; >+ >+ /* >+ * Scan backward and check the near @ra_max pages. >+ * The count here determines ra_size. >+ */ >+ 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?)
>+ >+ *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).
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.
--
Send instant messages to your online friends http://au.messenger.yahoo.com
- 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/
|  |