On 3/28/19 6:59 PM, Jerome Glisse wrote: >>>>>> [...] >>>>> Indeed I did not realize there is an hmm "pfn" until I saw this function: >>>>> >>>>> /* >>>>> * hmm_pfn_from_pfn() - create a valid HMM pfn value from pfn >>>>> * @range: range use to encode HMM pfn value >>>>> * @pfn: pfn value for which to create the HMM pfn >>>>> * Returns: valid HMM pfn for the pfn >>>>> */ >>>>> static inline uint64_t hmm_pfn_from_pfn(const struct hmm_range *range, >>>>> unsigned long pfn) >>>>> >>>>> So should this patch contain some sort of helper like this... maybe? >>>>> >>>>> I'm assuming the "hmm_pfn" being returned above is the device pfn being >>>>> discussed here? >>>>> >>>>> I'm also thinking calling it pfn is confusing. I'm not advocating a new type >>>>> but calling the "device pfn's" "hmm_pfn" or "device_pfn" seems like it would >>>>> have shortened the discussion here. >>>>> >>>> >>>> That helper is also use today by nouveau so changing that name is not that >>>> easy it does require the multi-release dance. So i am not sure how much >>>> value there is in a name change. >>>> >>> >>> Once the dust settles, I would expect that a name change for this could go >>> via Andrew's tree, right? It seems incredible to claim that we've built something >>> that effectively does not allow any minor changes! >>> >>> I do think it's worth some *minor* trouble to improve the name, assuming that we >>> can do it in a simple patch, rather than some huge maintainer-level effort. >> >> Change to nouveau have to go through nouveau tree so changing name means: Yes, I understand the guideline, but is that always how it must be done? Ben (+cc)? >> - release N add function with new name, maybe make the old function just >> a wrapper to the new function >> - release N+1 update user to use the new name >> - release N+2 remove the old name >> >> So it is do-able but it is painful so i rather do that one latter that now >> as i am sure people will then complain again about some little thing and it >> will post pone this whole patchset on that new bit. To avoid post-poning >> RDMA and bunch of other patchset that build on top of that i rather get >> this patchset in and then do more changes in the next cycle. >> >> This is just a capacity thing. > > Also for clarity changes to API i am doing in this patchset is to make > the ODP convertion easier and thus they bring a real hard value. Renaming > those function is esthetic, i am not saying it is useless, i am saying it > does not have the same value as those other changes and i would rather not > miss another merge window just for esthetic changes. > Agreed, that this minor point should not hold up this patch. thanks, -- John Hubbard NVIDIA
