Messages in this thread | | | Date | Fri, 04 Jan 2013 09:42:26 -0600 | From | Seth Jennings <> | Subject | Re: [PATCH 7/8] zswap: add to mm/ |
| |
On 01/03/2013 04:33 PM, Dan Magenheimer wrote: >> From: Seth Jennings [mailto:sjenning@linux.vnet.ibm.com] >> >> However, once the flushing code was introduced and could free an entry >> from the zswap_fs_store() path, it became necessary to add a per-entry >> refcount to make sure that the entry isn't freed while another code >> path was operating on it. > > Hmmm... doesn't the refcount at least need to be an atomic_t?
An entry's refcount is only ever changed under the tree lock, so making them atomic_t would be redundantly atomic.
I should add a comment to that effect though, including all elements that are protected by the tree lock which include: * the tree structure * the lru list * the per-entry refcounts
I'll put that change in the queue for v2.
> Also, how can you "free" any entry of an rbtree while another > thread is walking the rbtree? (Deleting an entry from an rbtree > causes rebalancing... afaik there is no equivalent RCU > implementation for rbtrees... not that RCU would necessarily > work well for this anyway.)
This also can't happen since a thread must obtain the tree lock before accessing or changing the tree.
Regarding RCU, I saw that some work had been done on RCU aware rbtree functions but they weren't ready yet.
> BTW, in case it appears otherwise, I'm trying to be helpful, not > critical. In the end, I think we are in agreement that in-kernel > compression is very important and that the frontswap (and/or > cleancache) interface(s) are the right way to identify compressible > data, and we are mostly arguing allocation and implementation details.
Yes. I'm always grateful for comments about the code :) At the very least, it rehashes the justifications for design decisions.
Thanks, Seth
| |