* [PATCH] drm/amdgpu: Fix minmax error @ 2022-11-24 21:19 Luben Tuikov 2022-11-24 22:04 ` James Zhu 2022-11-25 7:45 ` Christian König 0 siblings, 2 replies; 12+ messages in thread From: Luben Tuikov @ 2022-11-24 21:19 UTC (permalink / raw) To: AMD Graphics; +Cc: Felix Kuehling, Luben Tuikov, James Zhu Fix minmax compilation error by using min_t()/max_t(), of the assignment type. Cc: James Zhu <James.Zhu@amd.com> Cc: Felix Kuehling <Felix.Kuehling@amd.com> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c index 8a2e5716d8dba2..d22d14b0ef0c84 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, hmm_range->dev_private_owner = owner; do { - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); + hmm_range->end = min_t(typeof(hmm_range->end), + hmm_range->start + MAX_WALK_BYTE, + end); pr_debug("hmm range: start = 0x%lx, end = 0x%lx", hmm_range->start, hmm_range->end); /* Assuming 512MB takes maxmium 1 second to fault page address */ - timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL) * - HMM_RANGE_DEFAULT_TIMEOUT; + timeout = max_t(typeof(timeout), + (hmm_range->end - hmm_range->start) >> 29, + 1ULL); + timeout *= HMM_RANGE_DEFAULT_TIMEOUT; timeout = jiffies + msecs_to_jiffies(timeout); retry: base-commit: d5e8f4912061ad2e577b4909556e1364e2c2018e prerequisite-patch-id: 6024d0c36cae3e4a995a8fcf787b91f511a37486 -- 2.39.0.rc0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-24 21:19 [PATCH] drm/amdgpu: Fix minmax error Luben Tuikov @ 2022-11-24 22:04 ` James Zhu 2022-11-25 7:45 ` Christian König 1 sibling, 0 replies; 12+ messages in thread From: James Zhu @ 2022-11-24 22:04 UTC (permalink / raw) To: Luben Tuikov, AMD Graphics; +Cc: Felix Kuehling, James Zhu [-- Attachment #1: Type: text/plain, Size: 1718 bytes --] ThispatchisReviewed-by:JamesZhu<James.Zhu@amd.com> On 2022-11-24 16:19, Luben Tuikov wrote: > Fix minmax compilation error by using min_t()/max_t(), of the assignment type. > > Cc: James Zhu<James.Zhu@amd.com> > Cc: Felix Kuehling<Felix.Kuehling@amd.com> > Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") > Signed-off-by: Luben Tuikov<luben.tuikov@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c > index 8a2e5716d8dba2..d22d14b0ef0c84 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c > @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, > hmm_range->dev_private_owner = owner; > > do { > - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); > + hmm_range->end = min_t(typeof(hmm_range->end), > + hmm_range->start + MAX_WALK_BYTE, > + end); > > pr_debug("hmm range: start = 0x%lx, end = 0x%lx", > hmm_range->start, hmm_range->end); > > /* Assuming 512MB takes maxmium 1 second to fault page address */ > - timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL) * > - HMM_RANGE_DEFAULT_TIMEOUT; > + timeout = max_t(typeof(timeout), > + (hmm_range->end - hmm_range->start) >> 29, > + 1ULL); > + timeout *= HMM_RANGE_DEFAULT_TIMEOUT; > timeout = jiffies + msecs_to_jiffies(timeout); > > retry: > > base-commit: d5e8f4912061ad2e577b4909556e1364e2c2018e > prerequisite-patch-id: 6024d0c36cae3e4a995a8fcf787b91f511a37486 [-- Attachment #2: Type: text/html, Size: 2774 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-24 21:19 [PATCH] drm/amdgpu: Fix minmax error Luben Tuikov 2022-11-24 22:04 ` James Zhu @ 2022-11-25 7:45 ` Christian König 2022-11-25 7:56 ` Luben Tuikov 1 sibling, 1 reply; 12+ messages in thread From: Christian König @ 2022-11-25 7:45 UTC (permalink / raw) To: Luben Tuikov, AMD Graphics; +Cc: Felix Kuehling, James Zhu Am 24.11.22 um 22:19 schrieb Luben Tuikov: > Fix minmax compilation error by using min_t()/max_t(), of the assignment type. > > Cc: James Zhu <James.Zhu@amd.com> > Cc: Felix Kuehling <Felix.Kuehling@amd.com> > Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") > Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c > index 8a2e5716d8dba2..d22d14b0ef0c84 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c > @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, > hmm_range->dev_private_owner = owner; > > do { > - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); > + hmm_range->end = min_t(typeof(hmm_range->end), > + hmm_range->start + MAX_WALK_BYTE, > + end); Since end is a local variable I would strongly prefer to just have it use the correct type for it. Otherwise we might end up using something which doesn't work on all architectures. Regards, Christian. > > pr_debug("hmm range: start = 0x%lx, end = 0x%lx", > hmm_range->start, hmm_range->end); > > /* Assuming 512MB takes maxmium 1 second to fault page address */ > - timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL) * > - HMM_RANGE_DEFAULT_TIMEOUT; > + timeout = max_t(typeof(timeout), > + (hmm_range->end - hmm_range->start) >> 29, > + 1ULL); > + timeout *= HMM_RANGE_DEFAULT_TIMEOUT; > timeout = jiffies + msecs_to_jiffies(timeout); > > retry: > > base-commit: d5e8f4912061ad2e577b4909556e1364e2c2018e > prerequisite-patch-id: 6024d0c36cae3e4a995a8fcf787b91f511a37486 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-25 7:45 ` Christian König @ 2022-11-25 7:56 ` Luben Tuikov 2022-11-25 7:59 ` Christian König 0 siblings, 1 reply; 12+ messages in thread From: Luben Tuikov @ 2022-11-25 7:56 UTC (permalink / raw) To: Christian König, AMD Graphics; +Cc: Felix Kuehling, James Zhu On 2022-11-25 02:45, Christian König wrote: > > > Am 24.11.22 um 22:19 schrieb Luben Tuikov: >> Fix minmax compilation error by using min_t()/max_t(), of the assignment type. >> >> Cc: James Zhu <James.Zhu@amd.com> >> Cc: Felix Kuehling <Felix.Kuehling@amd.com> >> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") >> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >> index 8a2e5716d8dba2..d22d14b0ef0c84 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >> @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, >> hmm_range->dev_private_owner = owner; >> >> do { >> - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >> + hmm_range->end = min_t(typeof(hmm_range->end), >> + hmm_range->start + MAX_WALK_BYTE, >> + end); > > Since end is a local variable I would strongly prefer to just have it > use the correct type for it. > > Otherwise we might end up using something which doesn't work on all > architectures. They all appear to be "unsigned long". I thought, since we assign to hmm_range->end, we use that type. Would you prefer at the top of the function to define "timeout" and "end" as, typeof(hmm_range->end) end, timeout; Regards, Luben ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-25 7:56 ` Luben Tuikov @ 2022-11-25 7:59 ` Christian König 2022-11-25 8:33 ` Luben Tuikov 0 siblings, 1 reply; 12+ messages in thread From: Christian König @ 2022-11-25 7:59 UTC (permalink / raw) To: Luben Tuikov, AMD Graphics; +Cc: Felix Kuehling, James Zhu Am 25.11.22 um 08:56 schrieb Luben Tuikov: > On 2022-11-25 02:45, Christian König wrote: >> >> Am 24.11.22 um 22:19 schrieb Luben Tuikov: >>> Fix minmax compilation error by using min_t()/max_t(), of the assignment type. >>> >>> Cc: James Zhu <James.Zhu@amd.com> >>> Cc: Felix Kuehling <Felix.Kuehling@amd.com> >>> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") >>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>> index 8a2e5716d8dba2..d22d14b0ef0c84 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>> @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, >>> hmm_range->dev_private_owner = owner; >>> >>> do { >>> - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >>> + hmm_range->end = min_t(typeof(hmm_range->end), >>> + hmm_range->start + MAX_WALK_BYTE, >>> + end); >> Since end is a local variable I would strongly prefer to just have it >> use the correct type for it. >> >> Otherwise we might end up using something which doesn't work on all >> architectures. > They all appear to be "unsigned long". I thought, since we assign to > hmm_range->end, we use that type. Mhm, then why does the compiler complain here? As far as I can see "unsigned long" is correct here, but if we somehow have a typecast then something is not working as expected. Is MAX_WALK_BYTE maybe of signed type? > > Would you prefer at the top of the function to define "timeout" and "end" as, > typeof(hmm_range->end) end, timeout; Well for end that might make sense, but timeout is independent of the hmm range. Regards, Christian. > > Regards, > Luben > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-25 7:59 ` Christian König @ 2022-11-25 8:33 ` Luben Tuikov 2022-11-25 9:57 ` Christian König 0 siblings, 1 reply; 12+ messages in thread From: Luben Tuikov @ 2022-11-25 8:33 UTC (permalink / raw) To: Christian König, AMD Graphics; +Cc: Felix Kuehling, James Zhu On 2022-11-25 02:59, Christian König wrote: > Am 25.11.22 um 08:56 schrieb Luben Tuikov: >> On 2022-11-25 02:45, Christian König wrote: >>> >>> Am 24.11.22 um 22:19 schrieb Luben Tuikov: >>>> Fix minmax compilation error by using min_t()/max_t(), of the assignment type. >>>> >>>> Cc: James Zhu <James.Zhu@amd.com> >>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com> >>>> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") >>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- >>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>> index 8a2e5716d8dba2..d22d14b0ef0c84 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>> @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, >>>> hmm_range->dev_private_owner = owner; >>>> >>>> do { >>>> - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >>>> + hmm_range->end = min_t(typeof(hmm_range->end), >>>> + hmm_range->start + MAX_WALK_BYTE, >>>> + end); >>> Since end is a local variable I would strongly prefer to just have it >>> use the correct type for it. >>> >>> Otherwise we might end up using something which doesn't work on all >>> architectures. >> They all appear to be "unsigned long". I thought, since we assign to >> hmm_range->end, we use that type. > > Mhm, then why does the compiler complain here? Right... so MAX_WALK_BYTE is 2^36 ULL (diabolically defined as 64ULL<<30 :-) ), and this is why the minmax check complains. So, since the left-hand expression is unsigned long, i.e., hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); is, unsigned long = min(unsigned long long, unsigned long); The compiler complains. I'd really prefer MAX_WALK_BYTE be less than or equal to ULONG_MAX, and be defined as <literal>UL. I mean, why is everything in struct hmm_range "unsigned long", but we set a high limit of 10_0000_0000h for an end, and compare it to "end" to find the smaller? If our "end" could potentially be 10_0000_0000h then shouldn't the members in struct hmm_range be unsigned long long as well? And for the timeout, we have the (now) obvious, timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL); and I don't know why we necessarily need a "1ULL", when 1UL would do just fine, and then compilation passes for that statement. I can set this to 1UL, instead of using max_t(). Regards, Luben > > As far as I can see "unsigned long" is correct here, but if we somehow > have a typecast then something is not working as expected. > > Is MAX_WALK_BYTE maybe of signed type? > >> >> Would you prefer at the top of the function to define "timeout" and "end" as, >> typeof(hmm_range->end) end, timeout; > > Well for end that might make sense, but timeout is independent of the > hmm range. > > Regards, > Christian. > >> >> Regards, >> Luben >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-25 8:33 ` Luben Tuikov @ 2022-11-25 9:57 ` Christian König 2022-11-25 19:42 ` Luben Tuikov 0 siblings, 1 reply; 12+ messages in thread From: Christian König @ 2022-11-25 9:57 UTC (permalink / raw) To: Luben Tuikov, Christian König, AMD Graphics Cc: Felix Kuehling, James Zhu Am 25.11.22 um 09:33 schrieb Luben Tuikov: > On 2022-11-25 02:59, Christian König wrote: >> Am 25.11.22 um 08:56 schrieb Luben Tuikov: >>> On 2022-11-25 02:45, Christian König wrote: >>>> Am 24.11.22 um 22:19 schrieb Luben Tuikov: >>>>> Fix minmax compilation error by using min_t()/max_t(), of the assignment type. >>>>> >>>>> Cc: James Zhu <James.Zhu@amd.com> >>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com> >>>>> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") >>>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- >>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>> index 8a2e5716d8dba2..d22d14b0ef0c84 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>> @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, >>>>> hmm_range->dev_private_owner = owner; >>>>> >>>>> do { >>>>> - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >>>>> + hmm_range->end = min_t(typeof(hmm_range->end), >>>>> + hmm_range->start + MAX_WALK_BYTE, >>>>> + end); >>>> Since end is a local variable I would strongly prefer to just have it >>>> use the correct type for it. >>>> >>>> Otherwise we might end up using something which doesn't work on all >>>> architectures. >>> They all appear to be "unsigned long". I thought, since we assign to >>> hmm_range->end, we use that type. >> Mhm, then why does the compiler complain here? > Right... so MAX_WALK_BYTE is 2^36 ULL (diabolically defined as 64ULL<<30 :-) ), > and this is why the minmax check complains. > > So, since the left-hand expression is unsigned long, > i.e., > hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); > is, > unsigned long = min(unsigned long long, unsigned long); > The compiler complains. > > I'd really prefer MAX_WALK_BYTE be less than or equal to ULONG_MAX, That's not only a preference, but a must have. Otherwise the code maybe won't work as expected on 32bit architectures. > and be defined as <literal>UL. I mean, why is everything in struct hmm_range > "unsigned long", but we set a high limit of 10_0000_0000h for an end, and > compare it to "end" to find the smaller? If our "end" could potentially > be 10_0000_0000h then shouldn't the members in struct hmm_range be > unsigned long long as well? No, that the hmm range depends on the address space bits of the CPU is perfectly correct. Essentially this is just an userspace address range. Our problem here is that this code needs to work on both 32bit and 64bit systems. And on a 32bit system limiting the types wouldn't work correctly as far as I can see. So the compiler is complaining for rather good reasons and by using "min_t(UL" we just hide that instead of fixing the problem. I suggest to use "min_t(u64" instead. An intelligent compiler should even be capable of optimizing this away by looking at the input types on 32bit archs. > > And for the timeout, we have the (now) obvious, > > timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL); > > and I don't know why we necessarily need a "1ULL", when 1UL would do just fine, > and then compilation passes for that statement. I can set this to 1UL, instead > of using max_t(). I think just changing this to 1UL should be sufficient. Regards, Christian. > > Regards, > Luben > > >> As far as I can see "unsigned long" is correct here, but if we somehow >> have a typecast then something is not working as expected. >> >> Is MAX_WALK_BYTE maybe of signed type? >> >>> Would you prefer at the top of the function to define "timeout" and "end" as, >>> typeof(hmm_range->end) end, timeout; >> Well for end that might make sense, but timeout is independent of the >> hmm range. >> >> Regards, >> Christian. >> >>> Regards, >>> Luben >>> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-25 9:57 ` Christian König @ 2022-11-25 19:42 ` Luben Tuikov 2022-11-25 21:03 ` James Zhu 0 siblings, 1 reply; 12+ messages in thread From: Luben Tuikov @ 2022-11-25 19:42 UTC (permalink / raw) To: Christian König, Christian König, AMD Graphics Cc: Felix Kuehling, James Zhu On 2022-11-25 04:57, Christian König wrote: > > > Am 25.11.22 um 09:33 schrieb Luben Tuikov: >> On 2022-11-25 02:59, Christian König wrote: >>> Am 25.11.22 um 08:56 schrieb Luben Tuikov: >>>> On 2022-11-25 02:45, Christian König wrote: >>>>> Am 24.11.22 um 22:19 schrieb Luben Tuikov: >>>>>> Fix minmax compilation error by using min_t()/max_t(), of the assignment type. >>>>>> >>>>>> Cc: James Zhu <James.Zhu@amd.com> >>>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") >>>>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- >>>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>>> index 8a2e5716d8dba2..d22d14b0ef0c84 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>>> @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, >>>>>> hmm_range->dev_private_owner = owner; >>>>>> >>>>>> do { >>>>>> - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >>>>>> + hmm_range->end = min_t(typeof(hmm_range->end), >>>>>> + hmm_range->start + MAX_WALK_BYTE, >>>>>> + end); >>>>> Since end is a local variable I would strongly prefer to just have it >>>>> use the correct type for it. >>>>> >>>>> Otherwise we might end up using something which doesn't work on all >>>>> architectures. >>>> They all appear to be "unsigned long". I thought, since we assign to >>>> hmm_range->end, we use that type. >>> Mhm, then why does the compiler complain here? >> Right... so MAX_WALK_BYTE is 2^36 ULL (diabolically defined as 64ULL<<30 :-) ), >> and this is why the minmax check complains. >> >> So, since the left-hand expression is unsigned long, >> i.e., >> hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >> is, >> unsigned long = min(unsigned long long, unsigned long); >> The compiler complains. >> >> I'd really prefer MAX_WALK_BYTE be less than or equal to ULONG_MAX, > > That's not only a preference, but a must have. Otherwise the code maybe > won't work as expected on 32bit architectures. Well, I don't know what to change MAX_WALK_BYTE to, and given the suggestion below to change to min_t(u64, ...), I wonder if messing with MAX_WALK_BYTE even matters--it wouldn't matter so long as the type in min_t() is u64. It's a macro at the moment. However, the LHS--struct hmm_range's members are all unsigned long and then we're essentially doing (unsigned long) = (u64), which on 32-bit is (u32) = (u64). Regards, Luben > >> and be defined as <literal>UL. I mean, why is everything in struct hmm_range >> "unsigned long", but we set a high limit of 10_0000_0000h for an end, and >> compare it to "end" to find the smaller? If our "end" could potentially >> be 10_0000_0000h then shouldn't the members in struct hmm_range be >> unsigned long long as well? > > No, that the hmm range depends on the address space bits of the CPU is > perfectly correct. Essentially this is just an userspace address range. > > Our problem here is that this code needs to work on both 32bit and 64bit > systems. And on a 32bit system limiting the types wouldn't work > correctly as far as I can see. > > So the compiler is complaining for rather good reasons and by using > "min_t(UL" we just hide that instead of fixing the problem. > > I suggest to use "min_t(u64" instead. An intelligent compiler should > even be capable of optimizing this away by looking at the input types on > 32bit archs. > >> >> And for the timeout, we have the (now) obvious, >> >> timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL); >> >> and I don't know why we necessarily need a "1ULL", when 1UL would do just fine, >> and then compilation passes for that statement. I can set this to 1UL, instead >> of using max_t(). > > I think just changing this to 1UL should be sufficient. > > Regards, > Christian. > >> >> Regards, >> Luben >> >> >>> As far as I can see "unsigned long" is correct here, but if we somehow >>> have a typecast then something is not working as expected. >>> >>> Is MAX_WALK_BYTE maybe of signed type? >>> >>>> Would you prefer at the top of the function to define "timeout" and "end" as, >>>> typeof(hmm_range->end) end, timeout; >>> Well for end that might make sense, but timeout is independent of the >>> hmm range. >>> >>> Regards, >>> Christian. >>> >>>> Regards, >>>> Luben >>>> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-25 19:42 ` Luben Tuikov @ 2022-11-25 21:03 ` James Zhu 2022-11-26 3:19 ` Luben Tuikov 0 siblings, 1 reply; 12+ messages in thread From: James Zhu @ 2022-11-25 21:03 UTC (permalink / raw) To: Luben Tuikov, Christian König, Christian König, AMD Graphics Cc: Felix Kuehling, James Zhu On 2022-11-25 14:42, Luben Tuikov wrote: > On 2022-11-25 04:57, Christian König wrote: >> >> Am 25.11.22 um 09:33 schrieb Luben Tuikov: >>> On 2022-11-25 02:59, Christian König wrote: >>>> Am 25.11.22 um 08:56 schrieb Luben Tuikov: >>>>> On 2022-11-25 02:45, Christian König wrote: >>>>>> Am 24.11.22 um 22:19 schrieb Luben Tuikov: >>>>>>> Fix minmax compilation error by using min_t()/max_t(), of the assignment type. >>>>>>> >>>>>>> Cc: James Zhu <James.Zhu@amd.com> >>>>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>>> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") >>>>>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- >>>>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>>>> index 8a2e5716d8dba2..d22d14b0ef0c84 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>>>> @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, >>>>>>> hmm_range->dev_private_owner = owner; >>>>>>> >>>>>>> do { >>>>>>> - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >>>>>>> + hmm_range->end = min_t(typeof(hmm_range->end), >>>>>>> + hmm_range->start + MAX_WALK_BYTE, >>>>>>> + end); >>>>>> Since end is a local variable I would strongly prefer to just have it >>>>>> use the correct type for it. >>>>>> >>>>>> Otherwise we might end up using something which doesn't work on all >>>>>> architectures. >>>>> They all appear to be "unsigned long". I thought, since we assign to >>>>> hmm_range->end, we use that type. >>>> Mhm, then why does the compiler complain here? >>> Right... so MAX_WALK_BYTE is 2^36 ULL (diabolically defined as 64ULL<<30 :-) ), >>> and this is why the minmax check complains. >>> >>> So, since the left-hand expression is unsigned long, >>> i.e., >>> hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >>> is, >>> unsigned long = min(unsigned long long, unsigned long); >>> The compiler complains. >>> >>> I'd really prefer MAX_WALK_BYTE be less than or equal to ULONG_MAX, >> That's not only a preference, but a must have. Otherwise the code maybe >> won't work as expected on 32bit architectures. > Well, I don't know what to change MAX_WALK_BYTE to, and given the suggestion > below to change to min_t(u64, ...), I wonder if messing with MAX_WALK_BYTE > even matters--it wouldn't matter so long as the type in min_t() is u64. > It's a macro at the moment. > > However, the LHS--struct hmm_range's members are all > unsigned long and then we're essentially doing (unsigned long) = (u64), > which on 32-bit is (u32) = (u64). [JZ]MAX_WALK_BYTE can be reduce to #define MAX_WALK_BYTE (2UL<<30) > > Regards, > Luben > >>> and be defined as <literal>UL. I mean, why is everything in struct hmm_range >>> "unsigned long", but we set a high limit of 10_0000_0000h for an end, and >>> compare it to "end" to find the smaller? If our "end" could potentially >>> be 10_0000_0000h then shouldn't the members in struct hmm_range be >>> unsigned long long as well? >> No, that the hmm range depends on the address space bits of the CPU is >> perfectly correct. Essentially this is just an userspace address range. >> >> Our problem here is that this code needs to work on both 32bit and 64bit >> systems. And on a 32bit system limiting the types wouldn't work >> correctly as far as I can see. >> >> So the compiler is complaining for rather good reasons and by using >> "min_t(UL" we just hide that instead of fixing the problem. >> >> I suggest to use "min_t(u64" instead. An intelligent compiler should >> even be capable of optimizing this away by looking at the input types on >> 32bit archs. >> >>> And for the timeout, we have the (now) obvious, >>> >>> timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL); >>> >>> and I don't know why we necessarily need a "1ULL", when 1UL would do just fine, >>> and then compilation passes for that statement. I can set this to 1UL, instead >>> of using max_t(). >> I think just changing this to 1UL should be sufficient. >> >> Regards, >> Christian. >> >>> Regards, >>> Luben >>> >>> >>>> As far as I can see "unsigned long" is correct here, but if we somehow >>>> have a typecast then something is not working as expected. >>>> >>>> Is MAX_WALK_BYTE maybe of signed type? >>>> >>>>> Would you prefer at the top of the function to define "timeout" and "end" as, >>>>> typeof(hmm_range->end) end, timeout; >>>> Well for end that might make sense, but timeout is independent of the >>>> hmm range. >>>> >>>> Regards, >>>> Christian. >>>> >>>>> Regards, >>>>> Luben >>>>> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-25 21:03 ` James Zhu @ 2022-11-26 3:19 ` Luben Tuikov 0 siblings, 0 replies; 12+ messages in thread From: Luben Tuikov @ 2022-11-26 3:19 UTC (permalink / raw) To: James Zhu, Christian König, Christian König, AMD Graphics Cc: Felix Kuehling, James Zhu On 2022-11-25 16:03, James Zhu wrote: > > On 2022-11-25 14:42, Luben Tuikov wrote: >> On 2022-11-25 04:57, Christian König wrote: >>> >>> Am 25.11.22 um 09:33 schrieb Luben Tuikov: >>>> On 2022-11-25 02:59, Christian König wrote: >>>>> Am 25.11.22 um 08:56 schrieb Luben Tuikov: >>>>>> On 2022-11-25 02:45, Christian König wrote: >>>>>>> Am 24.11.22 um 22:19 schrieb Luben Tuikov: >>>>>>>> Fix minmax compilation error by using min_t()/max_t(), of the assignment type. >>>>>>>> >>>>>>>> Cc: James Zhu <James.Zhu@amd.com> >>>>>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>>>> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") >>>>>>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++--- >>>>>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>>>>> index 8a2e5716d8dba2..d22d14b0ef0c84 100644 >>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c >>>>>>>> @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, >>>>>>>> hmm_range->dev_private_owner = owner; >>>>>>>> >>>>>>>> do { >>>>>>>> - hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >>>>>>>> + hmm_range->end = min_t(typeof(hmm_range->end), >>>>>>>> + hmm_range->start + MAX_WALK_BYTE, >>>>>>>> + end); >>>>>>> Since end is a local variable I would strongly prefer to just have it >>>>>>> use the correct type for it. >>>>>>> >>>>>>> Otherwise we might end up using something which doesn't work on all >>>>>>> architectures. >>>>>> They all appear to be "unsigned long". I thought, since we assign to >>>>>> hmm_range->end, we use that type. >>>>> Mhm, then why does the compiler complain here? >>>> Right... so MAX_WALK_BYTE is 2^36 ULL (diabolically defined as 64ULL<<30 :-) ), >>>> and this is why the minmax check complains. >>>> >>>> So, since the left-hand expression is unsigned long, >>>> i.e., >>>> hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end); >>>> is, >>>> unsigned long = min(unsigned long long, unsigned long); >>>> The compiler complains. >>>> >>>> I'd really prefer MAX_WALK_BYTE be less than or equal to ULONG_MAX, >>> That's not only a preference, but a must have. Otherwise the code maybe >>> won't work as expected on 32bit architectures. >> Well, I don't know what to change MAX_WALK_BYTE to, and given the suggestion >> below to change to min_t(u64, ...), I wonder if messing with MAX_WALK_BYTE >> even matters--it wouldn't matter so long as the type in min_t() is u64. >> It's a macro at the moment. >> >> However, the LHS--struct hmm_range's members are all >> unsigned long and then we're essentially doing (unsigned long) = (u64), >> which on 32-bit is (u32) = (u64). > [JZ]MAX_WALK_BYTE can be reduce to #define MAX_WALK_BYTE (2UL<<30) Hi James--okay, I'll prep up a patch. Regards, Luben ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/amdgpu: Fix minmax error @ 2022-11-26 5:25 Luben Tuikov 2022-11-26 14:00 ` Christian König 0 siblings, 1 reply; 12+ messages in thread From: Luben Tuikov @ 2022-11-26 5:25 UTC (permalink / raw) To: AMD Graphics; +Cc: Felix Kuehling, Luben Tuikov, James Zhu Fix minmax compilation error by using the correct constant and correct integer suffix. Cc: James Zhu <James.Zhu@amd.com> Cc: Felix Kuehling <Felix.Kuehling@amd.com> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c index 8a2e5716d8dba2..65715cb395d838 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c @@ -51,7 +51,7 @@ #include "amdgpu_amdkfd.h" #include "amdgpu_hmm.h" -#define MAX_WALK_BYTE (64ULL<<30) +#define MAX_WALK_BYTE (2UL << 30) /** * amdgpu_hmm_invalidate_gfx - callback to notify about mm change @@ -197,8 +197,8 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, hmm_range->start, hmm_range->end); /* Assuming 512MB takes maxmium 1 second to fault page address */ - timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL) * - HMM_RANGE_DEFAULT_TIMEOUT; + timeout = max((hmm_range->end - hmm_range->start) >> 29, 1UL); + timeout *= HMM_RANGE_DEFAULT_TIMEOUT; timeout = jiffies + msecs_to_jiffies(timeout); retry: base-commit: 9e95ce4c60631c1339204f8723008a715391f410 -- 2.39.0.rc0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/amdgpu: Fix minmax error 2022-11-26 5:25 Luben Tuikov @ 2022-11-26 14:00 ` Christian König 0 siblings, 0 replies; 12+ messages in thread From: Christian König @ 2022-11-26 14:00 UTC (permalink / raw) To: Luben Tuikov, AMD Graphics; +Cc: Felix Kuehling, James Zhu Am 26.11.22 um 06:25 schrieb Luben Tuikov: > Fix minmax compilation error by using the correct constant and correct integer > suffix. > > Cc: James Zhu <James.Zhu@amd.com> > Cc: Felix Kuehling <Felix.Kuehling@amd.com> > Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory") > Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c > index 8a2e5716d8dba2..65715cb395d838 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c > @@ -51,7 +51,7 @@ > #include "amdgpu_amdkfd.h" > #include "amdgpu_hmm.h" > > -#define MAX_WALK_BYTE (64ULL<<30) > +#define MAX_WALK_BYTE (2UL << 30) > > /** > * amdgpu_hmm_invalidate_gfx - callback to notify about mm change > @@ -197,8 +197,8 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier, > hmm_range->start, hmm_range->end); > > /* Assuming 512MB takes maxmium 1 second to fault page address */ > - timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL) * > - HMM_RANGE_DEFAULT_TIMEOUT; > + timeout = max((hmm_range->end - hmm_range->start) >> 29, 1UL); > + timeout *= HMM_RANGE_DEFAULT_TIMEOUT; > timeout = jiffies + msecs_to_jiffies(timeout); > > retry: > > base-commit: 9e95ce4c60631c1339204f8723008a715391f410 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-11-26 14:01 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-24 21:19 [PATCH] drm/amdgpu: Fix minmax error Luben Tuikov 2022-11-24 22:04 ` James Zhu 2022-11-25 7:45 ` Christian König 2022-11-25 7:56 ` Luben Tuikov 2022-11-25 7:59 ` Christian König 2022-11-25 8:33 ` Luben Tuikov 2022-11-25 9:57 ` Christian König 2022-11-25 19:42 ` Luben Tuikov 2022-11-25 21:03 ` James Zhu 2022-11-26 3:19 ` Luben Tuikov 2022-11-26 5:25 Luben Tuikov 2022-11-26 14:00 ` Christian König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.