Re: [PATCH v2 hmm 2/9] mm/hmm: return the fault type from hmm_pte_need_fault()
On Fri, Mar 27, 2020 at 05:00:14PM -0300, Jason Gunthorpe wrote: > From: Jason Gunthorpe > > Using two bools instead of flags return is not necessary and leads to > bugs. Returning a value is easier for the compiler to check and easier to > pass around the code flow. > > Convert the two bools into flags and push the change to all callers. > > Signed-off-by: Jason Gunthorpe Looks good, Reviewed-by: Christoph Hellwig ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 2/9] mm/hmm: return the fault type from hmm_pte_need_fault()
On Tue, Mar 24, 2020 at 08:27:12AM +0100, Christoph Hellwig wrote: > On Mon, Mar 23, 2020 at 10:14:50PM -0300, Jason Gunthorpe wrote: > > +enum { > > + HMM_NEED_FAULT = 1 << 0, > > + HMM_NEED_WRITE_FAULT = HMM_NEED_FAULT | (1 << 1), > > + HMM_NEED_ALL_BITS = HMM_NEED_FAULT | HMM_NEED_WRITE_FAULT, > > I have to say I find the compound version of HMM_NEED_WRITE_FAULT > way harder to understand than the logic in the previous version, > and would refer keeping separate bits here. > > Mostly beccause of statements like this: > > > + if ((required_fault & HMM_NEED_WRITE_FAULT) == HMM_NEED_WRITE_FAULT) { > > which seems rather weird. Okay, I checked it over, and there is one weird statement above but only one place that |'s them together, so it is overall simpler to split the enum. I'll keep the HMM_NEED_ALL_BITS, I think that purpose is clear enough. Thanks, Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 2/9] mm/hmm: return the fault type from hmm_pte_need_fault()
On Mon, Mar 23, 2020 at 10:14:50PM -0300, Jason Gunthorpe wrote: > +enum { > + HMM_NEED_FAULT = 1 << 0, > + HMM_NEED_WRITE_FAULT = HMM_NEED_FAULT | (1 << 1), > + HMM_NEED_ALL_BITS = HMM_NEED_FAULT | HMM_NEED_WRITE_FAULT, I have to say I find the compound version of HMM_NEED_WRITE_FAULT way harder to understand than the logic in the previous version, and would refer keeping separate bits here. Mostly beccause of statements like this: > + if ((required_fault & HMM_NEED_WRITE_FAULT) == HMM_NEED_WRITE_FAULT) { which seems rather weird. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx