On Mon, Dec 14, 2020 at 09:54:06AM -0800, Linus Torvalds wrote: > > I expected to hate it more, but it looks reasonable. Opencoded > > xas_for_each() smells bad, but... > > I think the open-coded xas_for_each() per se isn't a problem, but I > agree that the startup condition is a bit ugly. And I'm actually > personally more confused by why xas_retry() is needed here, bit not in > many other places. That is perhaps more obvious now that it shows up > twice. > > Adding Willy to the cc in case he has comments on that, and can > explain it to me in small words. > > [ https://lore.kernel.org/lkml/20201214160724.ewhjqoi32chheone@box/ > for context ] The xas_retry() is something I now regret, but haven't got annoyed enough by it yet to fix (also, other projects). It originated in the radix tree where we would get a radix_tree_node and then iterate over it in header macros. If we're holding the rcu_read_lock() and somebody else deletes an entry leaving the entry at index 0 as the only index in the tree, we tell the RCU readers to rewalk the tree from the top by putting a retry entry in place of the real entry. It's not entirely clear to me now why we did that. Just leave the entry alone and the RCU-walkers will see it, then the rest of the node is empty. As to why we need to do this in some places and not others; you can only see a retry entry if you're only protected by the RCU lock. If you're protected by the spinlock, you can't see any nodes which contain retry entries. But I now think we should just get rid of retry entries. Maybe I'm missing a good reason to keep them.
