<pet...@infradead.org> 于2020年9月11日周五 下午11:55写道: > > On Wed, Sep 09, 2020 at 05:09:31PM +0800, qianjun.ker...@gmail.com wrote: > > From: jun qian <qianjun.ker...@gmail.com> > > > > When get the pending softirqs, it need to process all the pending > > softirqs in the while loop. If the processing time of each pending > > softirq is need more than 2 msec in this loop, or one of the softirq > > will running a long time, according to the original code logic, it > > will process all the pending softirqs without wakeuping ksoftirqd, > > which will cause a relatively large scheduling delay on the > > corresponding CPU, which we do not wish to see. The patch will check > > the total time to process pending softirq, if the time exceeds 2 ms > > we need to wakeup the ksofirqd to aviod large sched delay. > > But what is all that unreadaable gibberish with pending_new_{flag,bit} ? > > Random comments below.. > > > > +#define MAX_SOFTIRQ_TIME_NS 2000000 > > 2*NSEC_PER_MSEC > > > > +DEFINE_PER_CPU(__u32, pending_new_flag); > > +DEFINE_PER_CPU(__u32, pending_next_bit); > > __u32 is for userspace ABI, this is not it, use u32 > > > +#define SOFTIRQ_PENDING_MASK ((1UL << NR_SOFTIRQS) - 1) > > + > > asmlinkage __visible void __softirq_entry __do_softirq(void) > > { > > - unsigned long end = jiffies + MAX_SOFTIRQ_TIME; > > + u64 end = sched_clock() + MAX_SOFTIRQ_TIME_NS; > > unsigned long old_flags = current->flags; > > int max_restart = MAX_SOFTIRQ_RESTART; > > struct softirq_action *h; > > bool in_hardirq; > > - __u32 pending; > > - int softirq_bit; > > + __u32 pending, pending_left, pending_new; > > + int softirq_bit, next_bit; > > + unsigned long flags; > > > > /* > > * Mask out PF_MEMALLOC as the current task context is borrowed for > > the > > @@ -277,10 +282,33 @@ asmlinkage __visible void __softirq_entry > > __do_softirq(void) > > > > h = softirq_vec; > > > > - while ((softirq_bit = ffs(pending))) { > > - unsigned int vec_nr; > > + next_bit = per_cpu(pending_next_bit, smp_processor_id()); > > + per_cpu(pending_new_flag, smp_processor_id()) = 0; > > __this_cpu_read() / __this_cpu_write() > > > + > > + pending_left = pending & > > + (SOFTIRQ_PENDING_MASK << next_bit); > > + pending_new = pending & > > + (SOFTIRQ_PENDING_MASK >> (NR_SOFTIRQS - next_bit)); > > The second mask is the inverse of the first. > > > + /* > > + * In order to be fair, we shold process the pengding bits by the > > + * last processing order. > > + */ > > + while ((softirq_bit = ffs(pending_left)) || > > + (softirq_bit = ffs(pending_new))) { > > int prev_count; > > + unsigned int vec_nr = 0; > > > > + /* > > + * when the left pengding bits have been handled, we should > > + * to reset the h to softirq_vec. > > + */ > > + if (!ffs(pending_left)) { > > + if (per_cpu(pending_new_flag, smp_processor_id()) == > > 0) { > > + h = softirq_vec; > > + per_cpu(pending_new_flag, smp_processor_id()) > > = 1; > > + } > > + } > > h += softirq_bit - 1; > > > > vec_nr = h - softirq_vec; > > @@ -298,17 +326,44 @@ asmlinkage __visible void __softirq_entry > > __do_softirq(void) > > preempt_count_set(prev_count); > > } > > h++; > > - pending >>= softirq_bit; > > + > > + if (ffs(pending_left)) > > This is the _third_ ffs(pending_left), those things are _expensive_ (on > some archs, see include/asm-generic/bitops/__ffs.h). > > > + pending_left >>= softirq_bit; > > + else > > + pending_new >>= softirq_bit; > > + > > + /* > > + * the softirq's action has been run too much time, > > + * so it may need to wakeup the ksoftirqd > > + */ > > + if (need_resched() && sched_clock() > end) { > > + /* > > + * Ensure that the remaining pending bits will be > > + * handled. > > + */ > > + local_irq_save(flags); > > + if (ffs(pending_left)) > > *fourth*... > > > + or_softirq_pending((pending_left << (vec_nr + > > 1)) | > > + pending_new); > > + else > > + or_softirq_pending(pending_new << (vec_nr + > > 1)); > > + local_irq_restore(flags); > > + per_cpu(pending_next_bit, smp_processor_id()) = > > vec_nr + 1; > > + break; > > + } > > } > > > > + /* reset the pending_next_bit */ > > + per_cpu(pending_next_bit, smp_processor_id()) = 0; > > + > > if (__this_cpu_read(ksoftirqd) == current) > > rcu_softirq_qs(); > > local_irq_disable(); > > > > pending = local_softirq_pending(); > > if (pending) { > > - if (time_before(jiffies, end) && !need_resched() && > > - --max_restart) > > + if (!need_resched() && --max_restart && > > + sched_clock() <= end) > > goto restart; > > > > wakeup_softirqd(); > > This really wants to be a number of separate patches; and I quickly lost > the plot in your code. Instead of cleaning things up, you're making an > even bigger mess of things. > > That said, I _think_ I've managed to decode what you want. See the > completely untested patches attached. > >
thanks a lot thank for your suggestion. I will rewrite the patch by following you sugestion, Useing multiple patches to clarify ideas.