ERROR: drivers: iscsi: iscsi_target.c

ERROR: drivers: iscsi: iscsi_target.c

[Test Bot]

Hi,

I automatically test (RC) kernel and caught ERROR word.
Please ignore, if its unimportant.

Kernel: 5.18-rc6
Arch: x86_64 (SMP)
Compiler: 7.5.0 (gcc)

Codebase Block:

void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
{
        int ord, cpu;
        cpumask_t conn_allowed_cpumask;

        cpumask_and(&conn_allowed_cpumask, iscsit_global->allowed_cpumask,
                    cpu_online_mask);

       cpumask_clear(conn->conn_cpumask);
        ord = conn->bitmap_id % cpumask_weight(&conn_allowed_cpumask);
        for_each_cpu(cpu, &conn_allowed_cpumask) {
                if (ord-- == 0) {
                        cpumask_set_cpu(cpu, conn->conn_cpumask);
                        return;
                }
        }
        dump_stack();
        cpumask_setall(conn->conn_cpumask);
}

Compiler  Log:

drivers/target/iscsi/iscsi_target_configfs.c: In function
‘lio_target_wwn_cpus_allowed_list_store’:
drivers/target/iscsi/iscsi_target_configfs.c:1157:1: warning: the
frame size of 1032 bytes is larger than 1024 bytes
[-Wframe-larger-than=]

drivers/target/iscsi/iscsi_target.c: In function ‘iscsit_thread_get_cpumask’:
drivers/target/iscsi/iscsi_target.c:3625:1: warning: the frame size of
1032 bytes is larger than 1024 bytes [-Wframe-larger-than=]

Re: ERROR: drivers: iscsi: iscsi_target.c

[Linus Torvalds]

On Thu, May 12, 2022 at 11:42 AM Test Bot <zgrieee@gmail.com> wrote:
>
> void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
> {
>         int ord, cpu;
>         cpumask_t conn_allowed_cpumask;

Yeah, that's not how you are supposed to use 'cpumask_t'

This is why we have CONFIG_CPUMASK_OFFSTACK and 'cpumask_var_t', so
that the pattern is

        cpumask_var_t mask;

        if (!alloc_cpumask_var(&mask, GFP_KERNEL))
                return -ENOMEM;
        ... use 'mask' here as a  ...
        free_cpumask_var(mask);

and if the cpumask is small, it's allocated on the stack (and that
'alloc_cpumask_var()' becomes a no-op) and if it's huge, it has a real
allocation so that the stack frame doesn't grow too big.

The problem seems to have been introduced in this merge window by
commit d72d827f2f26 ("scsi: target: Add iscsi/cpus_allowed_list in
configfs"), but I didn't really look any closer than a plain "git
blame", so it might have happened before that too.

I also didn't check whether there was some explicit reason why the
code couldn't allocate the cpumask this way.

Btw, it's worth noting that 'cpumask_t' is always the full static
compile-time NR_CPUS bits in size, but a dynamically allocated
'cpumask_var_t' is only nr_cpumask_bits in size (ie the actual maximum
on that machine, as opposed to the theoretical maximum size). So they
are *not* exactly the same kind of 'cpumask_t' pointer in the end, but
no sane code should care (ie you have to do something else wrong for
the alloc_cpumask_var() pattern to not work).

           Linus

Re: ERROR: drivers: iscsi: iscsi_target.c

[Rasmus Villemoes]

On 12/05/2022 20.42, Test Bot wrote:
> Hi,
> 
> I automatically test (RC) kernel and caught ERROR word.
> Please ignore, if its unimportant.
> 
> Kernel: 5.18-rc6
> Arch: x86_64 (SMP)
> Compiler: 7.5.0 (gcc)
> 
> Codebase Block:
> 
> void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
> {
>         int ord, cpu;
>         cpumask_t conn_allowed_cpumask;
> 
>         cpumask_and(&conn_allowed_cpumask, iscsit_global->allowed_cpumask,
>                     cpu_online_mask);
> 
>        cpumask_clear(conn->conn_cpumask);
>         ord = conn->bitmap_id % cpumask_weight(&conn_allowed_cpumask);
>         for_each_cpu(cpu, &conn_allowed_cpumask) {
>                 if (ord-- == 0) {
>                         cpumask_set_cpu(cpu, conn->conn_cpumask);
>                         return;
>                 }
>         }
>         dump_stack();
>         cpumask_setall(conn->conn_cpumask);
> }

Hm, looks like cpumask.h should also expose a wrapper for
bitmap_ord_to_pos ...

Rasmus

Re: ERROR: drivers: iscsi: iscsi_target.c

[Linus Torvalds]

On Sun, May 15, 2022 at 10:48 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> 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