Virtualization - Jan 2023 - [Nouveau] [PATCH 1/8] iommu: Add a gfp parameter to iommu_map()

On Fri, Jan 06, 2023 at 05:15:28PM +0000, Robin Murphy
wrote:
> On 2023-01-06 16:42, Jason Gunthorpe wrote:
> > The internal mechanisms support this, but instead of exposting the gfp
to
> > the caller it wrappers it into iommu_map() and iommu_map_atomic()
> > 
> > Fix this instead of adding more variants for GFP_KERNEL_ACCOUNT.
> 
> FWIW, since we *do* have two variants already, I think I'd have a mild
> preference for leaving the regular map calls as-is (i.e. implicit
> GFP_KERNEL), and just generalising the _atomic versions for the special
> cases.
I think it is just better to follow kernel convention and have
allocation functions include the GFP because it is a clear signal to
the user that there is an allocation hidden inside the API. The whole
point of gfp is not to have multitudes of every function for every
allocation mode.

There are not so many callers that it seems worth worrying about
removing the extra GFP_KERNEL argument.
> However, echoing the recent activity over on the DMA API side of things, I
> think it's still worth proactively constraining the set of permissible
> flags, lest we end up with more weird problems if stuff that doesn't
really
> make sense, like GFP_COMP or zone flags, manages to leak through (that may
> have been part of the reason for having the current wrappers rather than a
> bare gfp argument in the first place, I forget now).
Yeah, that can be done

Thanks,
Jason

On Fri, Jan 06, 2023 at 01:24:11PM -0400, Jason Gunthorpe
wrote:
> I think it is just better to follow kernel convention and have
> allocation functions include the GFP because it is a clear signal to
> the user that there is an allocation hidden inside the API. The whole
> point of gfp is not to have multitudes of every function for every
> allocation mode.
Well, having GFP parameters is not a strict kernel convention. There are
places doing it differently and have sleeping and atomic variants of
APIs. I have to say I like the latter more. But given that this leads to
an invasion of API functions here which all do the same under the hood, I
agree it is better to go with a GFP parameter here.

Regards,

	Joerg

On Fri, Jan 20, 2023 at 10:24:55AM +0100, Joerg Roedel
wrote:
> On Fri, Jan 06, 2023 at 01:24:11PM -0400, Jason Gunthorpe wrote:
> > I think it is just better to follow kernel convention and have
> > allocation functions include the GFP because it is a clear signal to
> > the user that there is an allocation hidden inside the API. The whole
> > point of gfp is not to have multitudes of every function for every
> > allocation mode.
> 
> Well, having GFP parameters is not a strict kernel convention. There are
> places doing it differently and have sleeping and atomic variants of
> APIs. I have to say I like the latter more. But given that this leads to
> an invasion of API functions here which all do the same under the hood, I
> agree it is better to go with a GFP parameter here.
Ok, I think we are done with this series, I'll stick it in linux-next
for a bit and send you a PR so the trees stay in sync

Thanks,
Jason