Re: ERROR: drivers: iscsi: iscsi_target.c

[Linus Torvalds]

On Sun, May 15, 2022 at 10:48 PM Rasmus Villemoes
<rasmus.villemoes@xxxxxxxxx> wrote:
>
> Hm, looks like cpumask.h should also expose a wrapper for
> bitmap_ord_to_pos ...

I'd rather delete bitmap_ord_to_pos() entirely. We have something like
two uses of it, and both are illegible.

There's a comment about a possible third use, but that one says that
it's not using it because doing it differently is more efficient.

And honestly, that more efficient non-ord_to_pos implementation is
actually a lot more legible to any sane person too.

I think it's a huge mistake to create these kinds of obscure "helper"
functions that have odd semantics that almost nobody needs. It only
results in code that is completely obscure, because nobody is used to
those very unusual functions, so to understand the user you almost
have to go look at what the heck the helper function does.

That "bitmap_onto()" function (another crazy helper) is a great
example. Not only doesn't it use that ord_to_pos helper, the "helpful"
comments that describes the algorithm in terms of it is just much
harder to understand than the actual code.

NONE of those functions should exist at all, imho. That
"bitmap_onto()" function is some really specialized stuff that is used
in *one* single place, where it implements *another* really
specialized helper function, and that *other* helper function also has
exactly use use.

That should have clued people in.

I really think those functions should go away, and just be folded into
mpol_relative_nodemask(), where the semantics for it can actually be
explained.

Helper functions with one single use-case shouldn't be helper
fucntions. They should be inlines in the one single file that uses
them. Trying to make up abstraction layers is not "helping", it's just
making code harder to read, and trying to convince people to use
specialized helpers just causes more cognitive load.

                   Linus