Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeul...@suse.com> > Subject: Re: [PATCH v4 09/17] xen/arm: introduce a helper to parse device > tree NUMA distance map > > 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. Kind regards, Henry > > Jan > > > [1] https://lists.xenproject.org/archives/html/xen-devel/2021- > 08/msg01175.html > > > > Kind regards, > > Henry
