On Thu, Jan 11, 2018 at 10:22:58PM -0800, Eric Dumazet wrote:
> > asmlinkage __visible void __softirq_entry __do_softirq(void)
> > {
> > - unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
> > + struct softirq_stat *sstat = this_cpu_ptr(&softirq_stat_cpu);
> > unsigned long old_flags = current->flags;
> > - int max_restart = MAX_SOFTIRQ_RESTART;
> > struct softirq_action *h;
> > bool in_hardirq;
> > - __u32 pending;
> > + __u32 pending, overrun = 0;
> > int softirq_bit;
> >
> > /*
> > @@ -262,6 +273,7 @@ asmlinkage __visible void __softirq_entry
> > __do_softirq(void)
> > __local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);
> > in_hardirq = lockdep_softirq_start();
> >
> > + memzero_explicit(sstat, sizeof(*sstat));
>
> If you clear sstat here, it means it does not need to be a per cpu
> variable, but an automatic one (defined on the stack)
That's right. But I thought it was bit large for the stack:
struct {
u64 time;
u64 count;
} [NR_SOFTIRQS]
although arguably we are either using softirq stack or a fresh task one.
>
> I presume we need a per cpu var to track cpu usage on last time window.
>
> ( typical case of 99,000 IRQ per second, one packet delivered per IRQ,
> 10 usec spent per packet)
So should I account, like, per vector stats in a jiffy window for example? And
apply
the limits on top of that?
>
>
>
> > restart:
> > /* Reset the pending bitmask before enabling irqs */
> > set_softirq_pending(0);
> > @@ -271,8 +283,10 @@ asmlinkage __visible void __softirq_entry
> > __do_softirq(void)
> > h = softirq_vec;
> >
> > while ((softirq_bit = ffs(pending))) {
> > + struct vector_stat *vstat;
> > unsigned int vec_nr;
> > int prev_count;
> > + u64 startime;
> >
> > h += softirq_bit - 1;
> >
> > @@ -280,10 +294,18 @@ asmlinkage __visible void __softirq_entry
> > __do_softirq(void)
> > prev_count = preempt_count();
> >
> > kstat_incr_softirqs_this_cpu(vec_nr);
> > + vstat = &sstat->stat[vec_nr];
> >
> > trace_softirq_entry(vec_nr);
> > + startime = local_clock();
> > h->action(h);
> > + vstat->time += local_clock() - startime;
>
> You might store local_clock() in a variable, so that we do not call
> local_clock() two times per ->action() called.
So you mean I keep the second local_clock() call for the next first call in the
while
iteration, right? Yep that sounds possible.
>
>
> > + vstat->count++;
> > trace_softirq_exit(vec_nr);
> > +
> > + if (vstat->time > MAX_SOFTIRQ_TIME || vstat->count >
> > MAX_SOFTIRQ_RESTART)
>
> If we trust local_clock() to be precise enough, we do not need to
> track vstat->count anymore.
That's what I was thinking. Should I keep MAX_SOFTIRQ_TIME to 2 ms BTW? It
looks a bit long
to me.
Thanks.
