On Mar 28, 2020, at 5:15 PM, George Spelvin <lkml@xxxxxxx> wrote:
>
> On Sat, Mar 28, 2020 at 04:56:17PM -0600, Andreas Dilger wrote:
>>
>> So I think the current patch is fine. The for-loop construct of
>> using "++g == ngroups && (g = 0)" to wrap "g" around is new to me,
>> but looks correct.
>>
>> Reviewed-by: Andreas Dilger <adilger@xxxxxxxxx>
>
> Thank you. Standing back and looking from higher altitude, I missed
> a second modulo at fallback_retry: which should be made consistent,
> so I need a one re-spin.
>
> Also, we could, if desired, eliminate the i variable entirely
> using the fact that we have a copy of the starting position cached
> in parent_group. I.e.
>
> g = parent_group = reciprocal_scale(grp, ngroups);
> - for (i = 0; i < ngroups; i++, ++g == ngroups && (g = 0)) {
> + do {
> ...
> - }
> + if (++g == ngroups)
> + g = 0;
> + } while (g != parent_group);
>
> Or perhaps the following would be simpler, replacing the modulo
> with a conditional subtract:
>
> - g = parent_group = reciprocal_scale(grp, ngroups);
> + parent_group = reciprocal_scale(grp, ngroups);
> - for (i = 0; i < ngroups; i++, ++g == ngroups && (g = 0)) {
> + for (i = 0; i < ngroups; i++) {
> + g = parent_group + i;
> + if (g >= ngroups)
> + g -= ngroups;
>
> The conditional branch starts out always false, and ends up always true,
> but except for a few bobbles when it switches, branch prediction should
> handle it very well.
>
> Any preference?
I was looking at whether we could use a for-loop without "i"? Something like:
for (g = parent_group + 1; g != parent_group; ++g >= ngroups && (g = 0))
The initial group is parent_group + 1, to avoid special-casing when the
initial parent_group = 0 (which would prevent the loop from terminating).
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
