On Wed, 10 Jul 2024, Usama Arif wrote: > On 10/07/2024 21:49, Hugh Dickins wrote: > > It's a long time since I was active hereabouts, but the bot report > > and your flurry of updates make me think that you should step back, > > slow down, and look more carefully at the precedents here. > > > > IIRC, the main problem is that parts of the swap_info_struct can > > still be in use from before while you're wanting to set up new values. > > Observe how alloc_swap_info() may return a fresh or an old allocation. > > Observe how enable_swap_info() is called after getting swapon_mutex > > late in swapon(), once past all possiblities of error. > > > > I expect that your new zeromap needs to be taking the same care as is > > taken with swap_map and cluster_info: to be safe, follow their example > > in both swapon() and swapoff(). > > > > Hugh > > Thanks, yeah sent too many in quick succession :). Will be more careful next time. > > Both the 2nd and 3rd version are careful to solve the problem of using old allocation > which you described. > > The 2nd one takes care of it in the same way as swap_map. It didn't look like that: it set "p->zeromap = zeromap" as soon as zeromap had been allocated; whereas swap_map is only installed later via enable_swap_info(). > > But I believe its unnecessary to do all that change in the 2nd version, when you can just > set it to NULL after kvfree, which is a much smaller change and takes care of reusing old > allocation equally well. It's possible that there's a good reason why zeromap does not need the same care as swap_map; and it's possible that things have changed down the years and swap_map itself doesn't even need all that care. But you haven't persuaded me: I repeat, step back, slow down, think carefully about why the existing sequence is as it is (and please don't respond without doing so). Hugh
