On Tue, Dec 17, 2019 at 08:31:59AM -0800, Alexander Duyck wrote: > > > > I think you recently switched to using an atomic variable for maintaining page > > > > reporting status as I was doing in v12. > > > > Which is good, as we will not have a disagreement on it now. > > > > > > There is still some differences between our approaches if I am not > > > mistaken. Specifically I have code in place so that any requests to report > > > while we are actively working on reporting will trigger another pass being > > > scheduled after we completed. I still believe you were lacking any logic > > > like that as I recall. > > > > > > > Yes, I was specifically referring to the atomic state variable. > > Though I am wondering if having an atomic variable to track page reporting state > > is better than having a page reporting specific unsigned long flag, which we can > > manipulate via __set_bit() and __clear_bit(). > > So the reason for using an atomic state variable is because I only really > have 3 possible states; idle, active, and requested. It allows for a > pretty simple state machine as any transition from idle indicates that we > need to schedule the worker, transition from requested to active when the > worker starts, and if at the end of a pass if we are still in the active > state it means we can transition back to idle, otherwise we reschedule the > worker. > > In order to do the same sort of thing using the bitops would require at > least 2 bits. In addition with the requirement that I cannot use the zone > lock for protection of the state I cannot use the non-atomic versions of > things such as __set_bit and __clear_bit so they would require additional > locking protections. > I completely agree with this. I had pointed out in an earlier review that expanding the scope of the zone lock was inappropriate, the non-atomic operations on separate flags potentially misses updates and in general, I prefer the atomic variable approach a lot more than the previous zone->flag based approach. -- Mel Gorman SUSE Labs
