Am 02.07.2018 um 14:20 schrieb Michal Hocko: > On Mon 02-07-18 14:13:42, Christian König wrote: >> Am 02.07.2018 um 13:54 schrieb Michal Hocko: >>> On Mon 02-07-18 11:14:58, Christian König wrote: >>>> Am 27.06.2018 um 09:44 schrieb Michal Hocko: >>>>> This is the v2 of RFC based on the feedback I've received so far. The >>>>> code even compiles as a bonus ;) I haven't runtime tested it yet, mostly >>>>> because I have no idea how. >>>>> >>>>> Any further feedback is highly appreciated of course. >>>> That sounds like it should work and at least the amdgpu changes now look >>>> good to me on first glance. >>>> >>>> Can you split that up further in the usual way? E.g. adding the blockable >>>> flag in one patch and fixing all implementations of the MMU notifier in >>>> follow up patches. >>> But such a code would be broken, no? Ignoring the blockable state will >>> simply lead to lockups until the fixup parts get applied. >> Well to still be bisect-able you only need to get the interface change in >> first with fixing the function signature of the implementations. > That would only work if those functions return -AGAIN unconditionally. > Otherwise they would pretend to not block while that would be obviously > incorrect. This doesn't sound correct to me. > >> Then add all the new code to the implementations and last start to actually >> use the new interface. >> >> That is a pattern we use regularly and I think it's good practice to do >> this. > But we do rely on the proper blockable handling. Yeah, but you could add the handling only after you have all the implementations in place. Don't you? >>> Is the split up really worth it? I was thinking about that but had hard >>> times to end up with something that would be bisectable. Well, except >>> for returning -EBUSY until all notifiers are implemented. Which I found >>> confusing. >> It at least makes reviewing changes much easier, cause as driver maintainer >> I can concentrate on the stuff only related to me. >> >> Additional to that when you cause some unrelated side effect in a driver we >> can much easier pinpoint the actual change later on when the patch is >> smaller. >> >>>> This way I'm pretty sure Felix and I can give an rb on the amdgpu/amdkfd >>>> changes. >>> If you are worried to give r-b only for those then this can be done even >>> for larger patches. Just make your Reviewd-by more specific >>> R-b: name # For BLA BLA >> Yeah, possible alternative but more work for me when I review it :) > I definitely do not want to add more work to reviewers and I completely > see how massive "flag days" like these are not popular but I really > didn't find a reasonable way around that would be both correct and > wouldn't add much more churn on the way. So if you really insist then I > would really appreciate a hint on the way to achive the same without any > above downsides. Well, I don't insist on this. It's just from my point of view that this patch doesn't needs to be one patch, but could be split up. Could be that I just don't know the code or the consequences of adding that well enough to really judge. Christian.
