On 11/20/2014 01:50 PM, Tejun Heo wrote: > Hello, Boaz. > > On Thu, Nov 20, 2014 at 12:42:53PM +0200, Boaz Harrosh wrote: >> if I understand correctly the motivation here is that the allocation >> of the internal element is done GFP_KERNEL at this call >> >> Then the add() below can be under the spin_lock. >> >> So why don't you just return an element here to caller and give it to >> add below. No Preemption-disable, no percpu variable, simple. Like: > > Hmmm... mostly because preloading is more convenient and but also > because it provides better separation from internal implementation > details. e.g. This may be implemented using a different data > structure (e.g. bonsai tree) Two things: 1. This can be easily hidden by returning a none defined type which internals are only known to the implementation so even if you change the implementation users need not change. Like just a (void *) but it is better to be type-full like: struct pset_new; struct pset_new *pset_preload() And the internals of struct pset_new is only known to implementation 2. Obfuscation: Currently this is the proposed implementation if jugging by the previous imp it is good for 15 years. Else since when are we afraid to change two users? > which may require differing number of new > elements even on success. With the scheme you're describing, the > operation would be constantly allocating and freeing memory areas > (which may be multiple) unnecessarily. Actually with my proposed change to "the code you submitted here" there are *less* unnecessary allocations. In both our imp we have a waste when element already exist in the tree, and your imp already waists an allocation in every pset_preload() And again you are talking about a future undefined "what if", let us look at the very sound imp you proposed here with rbtree and do the best we can with that one. > > One thing which is debatable is how to handle preloading errors. We > can have the preload fail and then assume that the later insertion > won't fail with -ENOMEM (often through BUG/WARN_ON()); however, it > often, but not always, is that those insertion operations may fail > with different error codes too and requires error handling anyway, Again Theoretical. With your current code the only failing I see from add() is allocation, so with my imp it will never fail. One thing good with embedded list_heads is the void returning add. And so with my proposition: void returning add. When some new imp will be needed we can cross the bridge then. For now you have convinced me that an rbtree is good, and I want to solve the preemption-disable, none interrupt ugliness, per-cpu vars, as well as the double alloc in the normal lots-of-free-memory case. > so > overall it seems better to defer the allocation error to the actual > insertion point. That one I did not understand. > It also makes conceptual sense. The preloading > simply upgrades the allocation mask the insertion operation uses. > How is "upgrades", better then "always have the best mask" > Thanks. > Thanks Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
