Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200915115609.85106-1-qianjun.kernel@gmail.com>
 <20200915115609.85106-5-qianjun.kernel@gmail.com> <878scz89tl.fsf@nanos.tec.linutronix.de>
In-Reply-To: <878scz89tl.fsf@nanos.tec.linutronix.de>
From:   jun qian <qianjun.kernel@gmail.com>
Date:   Sat, 26 Sep 2020 10:00:36 +0800
Message-ID: <CAKc596+it+Cf2HuaeqM3kaFR4KTM7LdKcP3cMXC6AeWrixhLow@mail.gmail.com>
Subject: Re: [PATCH V7 4/4] softirq: Allow early break the softirq processing loop
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     peterz@infradead.org, will@kernel.org, luto@kernel.org,
        linux-kernel@vger.kernel.org, Yafang Shao <laoar.shao@gmail.com>,
        qais.yousef@arm.com, Uladzislau Rezki <urezki@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Thomas Gleixner <tglx@linutronix.de> =E4=BA=8E2020=E5=B9=B49=E6=9C=8824=E6=
=97=A5=E5=91=A8=E5=9B=9B =E4=B8=8B=E5=8D=8811:37=E5=86=99=E9=81=93=EF=BC=9A
>
> On Tue, Sep 15 2020 at 19:56, qianjun kernel wrote:
> >
> > +#define SOFTIRQ_PENDING_MASK ((1UL << NR_SOFTIRQS) - 1)
> >
> > +/*
> > + * The pending_next_bit is recorded for the next processing order when
> > + * the loop is broken. This per cpu variable is to solve the following
> > + * scenarios:
> > + * Assume bit 0 and 1 are pending when the processing starts. Now it
> > + * breaks out after bit 0 has been handled and stores back bit 1 as
> > + * pending. Before ksoftirqd runs bit 0 gets raised again. ksoftirqd
> > + * runs and handles bit 0, which takes more than the timeout. As a
> > + * result the bit 0 processing can starve all other softirqs.
> > + *
> > + * so we need the pending_next_bit to record the next process order.
> > + */
> > +DEFINE_PER_CPU(u32, pending_next_bit);
>
> static if at all.
>
> > +
> >  asmlinkage __visible void __softirq_entry __do_softirq(void)
> >  {
> >       u64 start =3D sched_clock();
> > @@ -261,8 +277,11 @@ asmlinkage __visible void __softirq_entry __do_sof=
tirq(void)
> >       unsigned int max_restart =3D MAX_SOFTIRQ_RESTART;
> >       struct softirq_action *h;
> >       unsigned long pending;
> > +     unsigned long pending_left, pending_again;
> >       unsigned int vec_nr;
> >       bool in_hardirq;
> > +     int next_bit;
> > +     unsigned long flags;
> >
> >       /*
> >        * Mask out PF_MEMALLOC as the current task context is borrowed f=
or the
> > @@ -283,25 +302,66 @@ asmlinkage __visible void __softirq_entry __do_so=
ftirq(void)
> >
> >       local_irq_enable();
> >
> > -     for_each_set_bit(vec_nr, &pending, NR_SOFTIRQS) {
> > -             int prev_count;
> > -
> > -             __clear_bit(vec_nr, &pending);
> > -
> > -             h =3D softirq_vec + vec_nr;
> > -
> > -             prev_count =3D preempt_count();
> > -
> > -             kstat_incr_softirqs_this_cpu(vec_nr);
> > +     /*
> > +      * pending_left means that the left bits unhandled when the loop =
is
> > +      * broken without finishing the vectors. These bits will be handl=
ed
> > +      * first in the next time. pending_again means that the new bits =
is
> > +      * generated in the other time. These bits should be handled afte=
r
> > +      * the pending_left bits have been handled.
> > +      *
> > +      * For example
> > +      * If the pending bits is 1101010110, and the loop is broken afte=
r
> > +      * the bit4 is handled. Then, the pending_next_bit will be 5, and
> > +      * the pending_left is 1101000000, the pending_again is 000000110=
.
> > +      */
>
> If you need such a comment to explain the meaning of your variables then
> you did something fundamentaly wrong.
>
> > +     next_bit =3D __this_cpu_read(pending_next_bit);
> > +     pending_left =3D pending &
> > +             (SOFTIRQ_PENDING_MASK << next_bit);
> > +     pending_again =3D pending &
> > +             (SOFTIRQ_PENDING_MASK >> (NR_SOFTIRQS - next_bit));
> > +
> > +     while (pending_left || pending_again) {
> > +             if  (pending_left) {
> > +                     pending =3D pending_left;
> > +                     pending_left =3D 0;
> > +             } else if (pending_again) {
> > +                     pending =3D pending_again;
> > +                     pending_again =3D 0;
> > +             } else
> > +                     break;
>
> Aside of lacking brackets how is that 'else' patch ever going to be
> reached?
>
> But TBH that whole patch is a completely unreviewable maze.
>
> This can be done without all this pending, pending_left, pending_again,
> pending_next_bit, next_bit convolution. It's inconsistent anyway:
>
> __do_softirq()
>
>         pending =3D 0x25;
>         next =3D 0;
>
>         for (...)
>             break after bit 0
>
>         =3D=3D> pending =3D=3D 0x24
>
>         =3D=3D> next =3D 2
>
> now on the next invocation
>
>        pending =3D 0x35;
>        next =3D 2;
>
>        So the processing order is 2, 4, 5, 0
>
> and there is nothing you can do about that with that approach.
>
> But the whole point is to ensure that the not yet processed bits are
> processed first.
>
> Find attached an updated series based on the original one from Peter
> with the authorship preserved, intact SOB chains and proper changelogs.
>
> The last one is new and addressing the starvation issue in a readable
> way.
>
> All of this is again completely untested.
>
> Thanks,
>
>         tglx
>

I will fix it and test. After test, i will send the patch again.

thanks