On 01/03/2013 04:33 PM, Dan Magenheimer wrote: >> From: Seth Jennings [mailto:sjenning@xxxxxxxxxxxxxxxxxx] >> >> 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>