MIME-Version: 1.0
References: <20230224135933.94104aeda1a0.Ie771c6a66d7d6c3cf67da5f3b0c66cea66fd514c@changeid>
 <dff29d82-9c4b-1933-c1c1-a3becf2a0f1f@lwfinger.net>
In-Reply-To: <dff29d82-9c4b-1933-c1c1-a3becf2a0f1f@lwfinger.net>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri, 24 Feb 2023 11:44:57 -0800
Message-ID: <CAHk-=whwDRefJJq0K8bXXSNY3-Zy8=Z3ZiKYh2mOOvfT-MqNhA@mail.gmail.com>
Subject: Re: [PATCH] wifi: wext: warn about usage only once
To:     Larry Finger <Larry.Finger@lwfinger.net>
Cc:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr>,
        Johannes Berg <johannes.berg@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Feb 24, 2023 at 7:45 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
>
> Although this patch will stop the log spamming that I see, it will not provide
> much information upstream for fixing the problems.

Generally, I think we've been happy with the "warn once".

Does it mean that if *multiple* different programs do this, we only
get a warning for the first one? Yes. But *users* don't actually care.
They can't do much about it.

And the people who *can* do things about it - ie the distro
maintainers - generally just need to be aware of, and fix, just one or
two cases anyway. If there are many more than that, then the warning
is bogus anyway, because deprecation is clearly the wrong thing.

So I personally think that "warn once" is much better than the "keep
some big array of names around" so that you can warn multiple times
patch I saw flying past.

That said, I would *not* object to "keep a single word of hashed bits"
around kind of model.  In fact, I've wanted something like that for
other situations.

So it might be interestring to have a "warn_once_per_comm()" thing,
that basically has a u32-sized "these hashes have been warned about",
and it does something like

 - hash current 'comm[]' contents

 - if the hash bit is not set in that single word, then warn, and set the bit.

IOW, it would turn a "warn_once()" into a "warn at most 32 times, and
at most once per comm[]" thing.

And it would use no more space for the "did I already warn" than just
a plain "warn_once()" does.

The hash doesn't need to be all that smart, and we would want it to be
fairly low-overhead. No need to make it some complicated
cryptographically secure hash. It could literally be something like

        u32 hash_comm(struct task_struct *t, unsigned int bits)
        {
                /* No need for locking, we're just hashing anyway */
                u32 val = READ_ONCE(*(u32 *)t->comm);
                return hash_32(val, bits);
        }

and then the "pr_warn_once_per_comm()" would do something like

        static u32 warned_mask;
        u32 mask = hash_comm(current, 5);
        if ((mask & warned_mask) == 0) {
                warned_mask |= mask;   // thread data race - we don't care
                pr_warn(..)
        }

which is all fairly simple and straightforward and not horribly inefficient.

You *could* improve on it further by having some kind of timed
rate-limiting, where every 24 hours you'd clear the warning mask, so
that you'd warn about these things once a day. That *can* be useful
for when people just don't notice the warning the first time around,
and "once a day" is not a horribly problem that fills up the logs like
the current situation does.

But again - I personally think even just a pr_warn_once() is likely
good enough. Because all I want is to not have that horrible
log-flushing behavior.

                 Linus