On 26.04.2023 09:36, Henry Wang wrote: >> -----Original Message----- >> From: Jan Beulich <jbeul...@suse.com> >> >> On 26.04.2023 08:29, Henry Wang wrote: >>>> -----Original Message----- >>>> From: Jan Beulich <jbeul...@suse.com> >>>> >>>>> + distance = dt_next_cell(1, &matrix); >>>> >>>> Upon second thought I checked what dt_next_cell() returns: You're silently >>>> truncating here and then ... >>>> >>>>> + /* Bi-directions are not set, set both */ >>>>> + numa_set_distance(from, to, distance); >>>>> + numa_set_distance(to, from, distance); >>>> >>>> ... here again. Is that really the intention? >>> >>> By hunting down the historical discussions I found that using dt_next_cell() >> is >>> what Julien suggested 2 years ago in the RFC series [1]. Given the >>> truncation >>> here is for node id (from/to) and distance which I am pretty sure will not >>> exceed 32-bit range, I think the silent truncation is safe. >> >> That discussion is orthogonal; the previously used dt_read_number() is no >> different in the regard I'm referring to. >> >>> However I understand your point here, the silent truncation is not ideal, so >>> I wonder if you have any suggestions to improve, do you think I should >> change >>> these variables to u64 or maybe I need to do the explicit type cast or any >>> better suggestions from you? Thanks! >> >> So one thing I overlooked is that by passing 1 as the first argument, you >> only request a 32-bit value. Hence there's no (silent) truncation then on >> the dt_next_cell() uses. But the numa_set_distance() calls still truncate >> to 8 bits. Adding explicit type casts won't help at all - truncation will >> remain as silent as it was before. However, numa_set_distance()'s first >> two arguments could easily become "unsigned int", resulting in its node >> related bounds checking to take care of all truncation issues. The >> "distance" parameter already is unsigned int, and is already being bounds >> checked against NUMA_NO_DISTANCE. > > Great points! Thanks for pointing the 8-bit truncation out. You are correct. > Somehow my impression of numa_set_distance()'s first two arguments are > already "unsigned int" so I missed this part...Sorry. > > In that case, I think I will add a check between "from, to" and MAX_NUMNODES > as soon as the values of "from" and "to" are populated by dt_next_cell(). > Hopefully this will address your concern.
While this would address by concern, I don't see why you want to repeat the checking that numa_set_distance() already does. Jan
