Hi Seth,
On wto, 2013-08-06 at 13:51 -0500, Seth Jennings wrote:
> I like the idea. I few things below. Also agree with Bob the
> s/rebalance/adjust/ for rebalance_lists().
OK.
> s/else if/if/ since the if above returns if true.
Sure.
> > + /* zbud_free() or zbud_alloc() */
> > + int freechunks = num_free_chunks(zhdr);
> > + list_add(&zhdr->buddy, &pool->unbuddied[freechunks]);
> > + } else {
> > + /* zbud_alloc() */
> > + list_add(&zhdr->buddy, &pool->buddied);
> > + }
> > + /* Add/move zbud page to beginning of LRU */
> > + if (!list_empty(&zhdr->lru))
> > + list_del(&zhdr->lru);
>
> We don't want to reinsert to the LRU list if we have called zbud_free()
> on a zbud page that previously had two buddies. This code causes the
> zbud page to move to the front of the LRU list which is not what we want.
Right, I'll fix it.
> > @@ -326,10 +370,10 @@ found:
> > void zbud_free(struct zbud_pool *pool, unsigned long handle)
> > {
> > struct zbud_header *zhdr;
> > - int freechunks;
> >
> > spin_lock(&pool->lock);
> > zhdr = handle_to_zbud_header(handle);
> > + BUG_ON(zhdr->last_chunks == 0 && zhdr->first_chunks == 0);
>
> Not sure we need this. Maybe, at most, VM_BUG_ON()?
Actually it is somehow a leftover after debugging so I don't mind
removing it completely.
> > @@ -411,11 +438,24 @@ int zbud_reclaim_page(struct zbud_pool *pool,
> > unsigned int retries)
> > return -EINVAL;
> > }
> > for (i = 0; i < retries; i++) {
> > + if (list_empty(&pool->lru)) {
> > + /*
> > + * LRU was emptied during evict calls in previous
> > + * iteration but put_zbud_page() returned 0 meaning
> > + * that someone still holds the page. This may
> > + * happen when some other mm mechanism increased
> > + * the page count.
> > + * In such case we succedded with reclaim.
> > + */
> > + return 0;
> > + }
> > zhdr = list_tail_entry(&pool->lru, struct zbud_header, lru);
> > + BUG_ON(zhdr->first_chunks == 0 && zhdr->last_chunks == 0);
>
> Again here.
I agree.
Thanks for comments,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
