On Sat, Dec 12, 2020 at 11:06:41PM +0100, Andrew Lunn wrote: > > + /* Complication created by the fact that addition has two phases, but > > + * deletion only has one phase, and we need reference counting. > > + * The strategy is to do the memory allocation in the prepare phase, > > + * but initialize the refcount in the commit phase. > > + * > > + * Have mdb | mdb has refcount > 0 | Commit phase | Resolution > > + * -------------+-----------------------+---------------+--------------- > > + * no | - | no | Alloc & > > proceed > > This does not look right. > > The point of the prepare phase is to allow all the different layers > involved to allocate whatever they need and to validate they can do > the requested action. Any layer can say, No, stop, i cannot do > this. The commit phase will then not happen. But that also means the > prepare phase should not do any state changes. So you should not be > proceeding here, just allocating.
Are you commenting based on the code, or just based on the comment? If just based on the comment, then yeah, sorry. I was limited to 80 characters, and I couldn't specify "proceed to what". It's just "proceed to call the prepare phase of the driver". Which is... normal and expected, and does not contradict what you said above. > And you need some way to cleanup the allocated memory when the commit > never happens because some other layer has said No! So this would be a fatal problem with the switchdev transactional model if I am not misunderstanding it. On one hand there's this nice, bubbly idea that you should preallocate memory in the prepare phase, so that there's one reason less to fail at commit time. But on the other hand, if "the commit phase might never happen" is even a remove possibility, all of that goes to trash - how are you even supposed to free the preallocated memory. Sorry, I don't think that there's any possibility for the commit phase to not happen as long as everybody is in agreement that the preparation phase went ok. If you look at the code, I even allocated the memory at preparation time _before_ calling into the driver, to ensure that we're not giving the driver the false impression that it gave switchdev the green light but the commit never came. If our allocation in the DSA core fails during the prepare phase, the prepare phase of the driver is not even called. That being said, please let me know if you spot bugs in the actual code. I tested it and it appeared to work ok (I also put debugging prints to make sure that the refcounting works ok and the entries are removed after all of them expire).
