Re: [PATCH 1/1] sched/fair:Reduce unnecessary check preempt in the sched tick

[jun qian]

Peter Zijlstra  于2021年4月13日周二 下午10:24写道：
>
> On Tue, Apr 13, 2021 at 09:18:42PM +0800, qianjun.ker...@gmail.com wrote:
> > From: jun qian 
> >
> > If it has been determined that the current cpu need resched in the
> > early stage of for_each_sched_entity, then there is no need to check
> > preempt in the subsequent se->parent entity_tick.
>
> Right, but does it actually do anything, except increase linecount?
>
> > Signed-off-by: jun qian 
> > ---
> >  kernel/sched/fair.c | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 1a68a0536add..c0d135100d54 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4352,8 +4352,13 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct 
> > sched_entity *curr)
> >  {
> >   unsigned long ideal_runtime, delta_exec;
> >   struct sched_entity *se;
> > + struct rq *rq = rq_of(cfs_rq);
> >   s64 delta;
> >
> > + /* If the TIF_NEED_RESCHED has been set, it is no need to check again 
> > */
> > + if (test_tsk_need_resched(rq->curr))
> > + return;
> > +
> >   ideal_runtime = sched_slice(cfs_rq, curr);
> >   delta_exec = curr->sum_exec_runtime - curr->prev_sum_exec_runtime;
> >   if (delta_exec > ideal_runtime) {
>
> Also, I think that's placed wrong; this way we can mis clear_buddies().

thanks, i will modify it in the next version

Re: [PATCH V2 1/1] mm:improve the performance during fork

[jun qian]

Andrew Morton  于2021年3月31日周三 下午1:44写道：
>
> On Mon, 29 Mar 2021 20:36:35 +0800 qianjun.ker...@gmail.com wrote:
>
> > From: jun qian 
> >
> > In our project, Many business delays come from fork, so
> > we started looking for the reason why fork is time-consuming.
> > I used the ftrace with function_graph to trace the fork, found
> > that the vm_normal_page will be called tens of thousands and
> > the execution time of this vm_normal_page function is only a
> > few nanoseconds. And the vm_normal_page is not a inline function.
> > So I think if the function is inline style, it maybe reduce the
> > call time overhead.
> >
> > I did the following experiment:
> >
> > use the bpftrace tool to trace the fork time :
> >
> > bpftrace -e 'kprobe:_do_fork/comm=="redis-server"/ {@st=nsecs;} \
> > kretprobe:_do_fork /comm=="redis-server"/{printf("the fork time \
> > is %d us\n", (nsecs-@st)/1000)}'
> >
> > no inline vm_normal_page:
> > result:
> > the fork time is 40743 us
> > the fork time is 41746 us
> > the fork time is 41336 us
> > the fork time is 42417 us
> > the fork time is 40612 us
> > the fork time is 40930 us
> > the fork time is 41910 us
> >
> > inline vm_normal_page:
> > result:
> > the fork time is 39276 us
> > the fork time is 38974 us
> > the fork time is 39436 us
> > the fork time is 38815 us
> > the fork time is 39878 us
> > the fork time is 39176 us
> >
> > In the same test environment, we can get 3% to 4% of
> > performance improvement.
> >
> > note:the test data is from the 4.18.0-193.6.3.el8_2.v1.1.x86_64,
> > because my product use this version kernel to test the redis
> > server, If you need to compare the latest version of the kernel
> > test data, you can refer to the version 1 Patch.
> >
> > We need to compare the changes in the size of vmlinux:
> >   inline   non-inline   diff
> > vmlinux size  9709248 bytes9709824 bytes-576 bytes
> >
>
> I get very different results with gcc-7.2.0:
>
> q:/usr/src/25> size mm/memory.o
>textdata bss dec hex filename
>   748983375  64   78337   13201 mm/memory.o-before
>   751193363  64   78546   132d2 mm/memory.o-after
>
> That's a somewhat significant increase in code size, and larger code
> size has a worsened cache footprint.
>
> Not that this is necessarily a bad thing for a function which is
> tightly called many times in succession as is vm__normal_page()
>
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -592,7 +592,7 @@ static void print_bad_pte(struct vm_area_struct *vma, 
> > unsigned long addr,
> >   * PFNMAP mappings in order to support COWable mappings.
> >   *
> >   */
> > -struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
> > +inline struct page *vm_normal_page(struct vm_area_struct *vma, unsigned 
> > long addr,
> >   pte_t pte)
> >  {
> >   unsigned long pfn = pte_pfn(pte);
>
> I'm a bit surprised this made any difference - rumour has it that
> modern gcc just ignores `inline' and makes up its own mind.  Which is
> why we added __always_inline.
>
the kernel code version: kernel-4.18.0-193.6.3.el8_2
gcc version 8.4.1 20200928 (Red Hat 8.4.1-1) (GCC)

and I made it again, got the results, and later i will test in the
latest version kernel with the new gcc.

757368576  vmlinux   inline
757381440  vmlinux   no inline

Re: [PATCH 1/1] mm:improve the performance during fork

[jun qian]

David Laight  于2020年12月23日周三 上午2:42写道：
>
> From: qianjun
> > Sent: 22 December 2020 12:19
> >
> > In our project, Many business delays come from fork, so
> > we started looking for the reason why fork is time-consuming.
> > I used the ftrace with function_graph to trace the fork, found
> > that the vm_normal_page will be called tens of thousands and
> > the execution time of this vm_normal_page function is only a
> > few nanoseconds. And the vm_normal_page is not a inline function.
> > So I think if the function is inline style, it maybe reduce the
> > call time overhead.
>
> Beware of taking timings from ftrace function trace.
> The cost of the tracing is significant.
>
> You can get sensible numbers if you only trace very specific
> functions.
> Slightly annoyingly the output format changes if you enable
> the function exit trace - useful for the timestamp.
> ISTR it is possible to get the process id traced if you fiddle
> with enough options.
>
> David
>

Thanks for your time

I have closed the ftrace when the test program is running. So the time
diff is without the
ftrace interference.And what does ISTR stand for :)  thanks.

> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 
> 1PT, UK
> Registration No: 1397386 (Wales)
>

Re: [PATCH 1/1] mm:improve the performance during fork

[jun qian]

Souptick Joarder  于2020年12月22日周二 下午11:08写道：
>
> On Tue, Dec 22, 2020 at 5:49 PM  wrote:
> >
> > From: jun qian 
> >
> > In our project, Many business delays come from fork, so
> > we started looking for the reason why fork is time-consuming.
> > I used the ftrace with function_graph to trace the fork, found
> > that the vm_normal_page will be called tens of thousands and
> > the execution time of this vm_normal_page function is only a
> > few nanoseconds. And the vm_normal_page is not a inline function.
> > So I think if the function is inline style, it maybe reduce the
> > call time overhead.
> >
> > I did the following experiment:
> >
> > I have wrote the c test code, pls ignore the memory leak :)
> > Before fork, I will malloc 4G bytes, then acculate the fork
> > time.
> >
> > int main()
> > {
> > char *p;
> > unsigned long long i=0;
> > float time_use=0;
> > struct timeval start;
> > struct timeval end;
> >
> > for(i=0; i > p = (char *)malloc(4096);
> > if (p == NULL) {
> > printf("malloc failed!\n");
> > return 0;
> > }
> > p[0] = 0x55;
> > }
> > gettimeofday(,NULL);
> > fork();
> > gettimeofday(,NULL);
> >
> > time_use=(end.tv_sec * 100 + end.tv_usec) -
> > (start.tv_sec * 100 + start.tv_usec);
> > printf("time_use is %.10f us\n",time_use);
> >
> > return 0;
> > }
> >
> > We need to compare the changes in the size of vmlinux, the time of
> > fork in inline and non-inline cases, and the vm_normal_page will be
> > called in many function. So we also need to compare this function's
> > size. For examples, the do_wp_page will call vm_normal_page, so I
> > also calculated it's size.
> >
> >   inline   non-inline   diff
> > vmlinux size  9709248 bytes9709824 bytes-576 bytes
> > fork time 23475ns  24638ns  -4.7%
>
> Do you have time diff for both parent and child process ?

yes, the child time diff and the parent time diff are almost same,
just like this, a.out is the test program.

./a.out
time_use is 23342.00 us
time_use is 23404.00 us

>
> > do_wp_page size   972  743  +229
> >
> > According to the above test data, I think inline vm_normal_page can
> > reduce fork execution time.
> >
> > Signed-off-by: jun qian 
> > ---
> >  mm/memory.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 7d608765932b..a689bb5d3842 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -591,7 +591,7 @@ static void print_bad_pte(struct vm_area_struct *vma, 
> > unsigned long addr,
> >   * PFNMAP mappings in order to support COWable mappings.
> >   *
> >   */
> > -struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
> > +inline struct page *vm_normal_page(struct vm_area_struct *vma, unsigned 
> > long addr,
> > pte_t pte)
> >  {
> > unsigned long pfn = pte_pfn(pte);
> > --
> > 2.18.2
> >
> >

Re: [RFC PATCH 4/4] sched, rt: support schedstat for RT sched class

[jun qian]

Yafang Shao  于2020年11月19日周四 上午11:55写道：
>
> We want to measure the latency of RT tasks in our production
> environment with schedstat facility, but currently schedstat is only
> supported for fair sched class. This patch enable it for RT sched class
> as well.
>
> The schedstat statistics are define in struct sched_entity, which is a
> member of struct task_struct, so we can resue it for RT sched class.
>
> The schedstat usage in RT sched class is similar with fair sched class,
> for example,
> fairRT
> enqueue update_stats_enqueue_fair   update_stats_enqueue_rt
> dequeue update_stats_dequeue_fair   update_stats_dequeue_rt
> put_prev_task   update_stats_wait_start update_stats_wait_start
> set_next_task   update_stats_wait_end   update_stats_wait_end
> show/proc/[pid]/sched   /proc/[pid]/sched
>
> The sched:sched_stats_* tracepoints can be used to trace RT tasks as
> well.
>
> Signed-off-by: Yafang Shao 
> ---
>  kernel/sched/rt.c| 61 
>  kernel/sched/sched.h |  2 ++
>  2 files changed, 63 insertions(+)
>
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index b9ec886702a1..a318236b7166 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1246,6 +1246,46 @@ void dec_rt_tasks(struct sched_rt_entity *rt_se, 
> struct rt_rq *rt_rq)
> dec_rt_group(rt_se, rt_rq);
>  }
>

Does the deadline schedule class should be considered also?

thanks

> +static inline void
> +update_stats_enqueue_rt(struct rq *rq, struct sched_entity *se,
> +   struct sched_rt_entity *rt_se, int flags)
> +{
> +   struct rt_rq *rt_rq = >rt;
> +
> +   if (!schedstat_enabled())
> +   return;
> +
> +   if (rt_se != rt_rq->curr)
> +   update_stats_wait_start(rq, se);
> +
> +   if (flags & ENQUEUE_WAKEUP)
> +   update_stats_enqueue_sleeper(rq, se);
> +}
> +
> +static inline void
> +update_stats_dequeue_rt(struct rq *rq, struct sched_entity *se,
> +   struct sched_rt_entity *rt_se, int flags)
> +{
> +   struct rt_rq *rt_rq = >rt;
> +
> +   if (!schedstat_enabled())
> +   return;
> +
> +   if (rt_se != rt_rq->curr)
> +   update_stats_wait_end(rq, se);
> +
> +   if ((flags & DEQUEUE_SLEEP) && rt_entity_is_task(rt_se)) {
> +   struct task_struct *tsk = rt_task_of(rt_se);
> +
> +   if (tsk->state & TASK_INTERRUPTIBLE)
> +   __schedstat_set(se->statistics.sleep_start,
> +   rq_clock(rq));
> +   if (tsk->state & TASK_UNINTERRUPTIBLE)
> +   __schedstat_set(se->statistics.block_start,
> +   rq_clock(rq));
> +   }
> +}
> +
>  /*
>   * Change rt_se->run_list location unless SAVE && !MOVE
>   *
> @@ -1275,6 +1315,7 @@ static void __enqueue_rt_entity(struct sched_rt_entity 
> *rt_se, unsigned int flag
> struct rt_prio_array *array = _rq->active;
> struct rt_rq *group_rq = group_rt_rq(rt_se);
> struct list_head *queue = array->queue + rt_se_prio(rt_se);
> +   struct task_struct *task = rt_task_of(rt_se);
>
> /*
>  * Don't enqueue the group if its throttled, or when empty.
> @@ -1288,6 +1329,8 @@ static void __enqueue_rt_entity(struct sched_rt_entity 
> *rt_se, unsigned int flag
> return;
> }
>
> +   update_stats_enqueue_rt(rq_of_rt_rq(rt_rq), >se, rt_se, flags);
> +
> if (move_entity(flags)) {
> WARN_ON_ONCE(rt_se->on_list);
> if (flags & ENQUEUE_HEAD)
> @@ -1307,7 +1350,9 @@ static void __dequeue_rt_entity(struct sched_rt_entity 
> *rt_se, unsigned int flag
>  {
> struct rt_rq *rt_rq = rt_rq_of_se(rt_se);
> struct rt_prio_array *array = _rq->active;
> +   struct task_struct *task = rt_task_of(rt_se);
>
> +   update_stats_dequeue_rt(rq_of_rt_rq(rt_rq), >se, rt_se, flags);
> if (move_entity(flags)) {
> WARN_ON_ONCE(!rt_se->on_list);
> __delist_rt_entity(rt_se, array);
> @@ -1374,6 +1419,7 @@ enqueue_task_rt(struct rq *rq, struct task_struct *p, 
> int flags)
> if (flags & ENQUEUE_WAKEUP)
> rt_se->timeout = 0;
>
> +   check_schedstat_required();
> enqueue_rt_entity(rt_se, flags);
>
> if (!task_current(rq, p) && p->nr_cpus_allowed > 1)
> @@ -1574,6 +1620,12 @@ static void check_preempt_curr_rt(struct rq *rq, 
> struct task_struct *p, int flag
>
>  static inline void set_next_task_rt(struct rq *rq, struct task_struct *p, 
> bool first)
>  {
> +   struct sched_rt_entity *rt_se = >rt;
> +   struct rt_rq *rt_rq = >rt;
> +
> +   if (on_rt_rq(>rt))
> +   update_stats_wait_end(rq, >se);
> +
> update_stats_curr_start(rq, >se);
>
> /* The running task is never 

[tip: sched/core] sched/fair: Improve the accuracy of sched_stat_wait statistics

[tip-bot2 for jun qian]

The following commit has been merged into the sched/core branch of tip:

Commit-ID: b9c88f752268383beff0d56e50d52b8ae62a02f8
Gitweb:
https://git.kernel.org/tip/b9c88f752268383beff0d56e50d52b8ae62a02f8
Author:jun qian 
AuthorDate:Thu, 15 Oct 2020 14:48:46 +08:00
Committer: Peter Zijlstra 
CommitterDate: Thu, 29 Oct 2020 11:00:28 +01:00

sched/fair: Improve the accuracy of sched_stat_wait statistics

When the sched_schedstat changes from 0 to 1, some sched se maybe
already in the runqueue, the se->statistics.wait_start will be 0.
So it will let the (rq_of(cfs_rq)) - se->statistics.wait_start)
wrong. We need to avoid this scenario.

Signed-off-by: jun qian 
Signed-off-by: Peter Zijlstra (Intel) 
Reviewed-by: Yafang Shao 
Link: https://lkml.kernel.org/r/20201015064846.19809-1-qianjun.ker...@gmail.com
---
 kernel/sched/fair.c |  9 +
 1 file changed, 9 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 290f9e3..b9368d1 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -906,6 +906,15 @@ update_stats_wait_end(struct cfs_rq *cfs_rq, struct 
sched_entity *se)
if (!schedstat_enabled())
return;
 
+   /*
+* When the sched_schedstat changes from 0 to 1, some sched se
+* maybe already in the runqueue, the se->statistics.wait_start
+* will be 0.So it will let the delta wrong. We need to avoid this
+* scenario.
+*/
+   if (unlikely(!schedstat_val(se->statistics.wait_start)))
+   return;
+
delta = rq_clock(rq_of(cfs_rq)) - 
schedstat_val(se->statistics.wait_start);
 
if (entity_is_task(se)) {

Re: [PATCH] sched/cputime: correct account of irqtime

[jun qian]

Pingfan Liu  于2020年10月12日周一 下午9:54写道：
>
> __do_softirq() may be interrupted by hardware interrupts. In this case,
> irqtime_account_irq() will account the time slice as CPUTIME_SOFTIRQ by
> mistake.
>
> By passing irqtime_account_irq() an extra param about either hardirq or
> softirq, irqtime_account_irq() can handle the above case.
>
> Signed-off-by: Pingfan Liu 
> Cc: Ingo Molnar 
> Cc: Peter Zijlstra 
> Cc: Juri Lelli 
> Cc: Vincent Guittot 
> Cc: Dietmar Eggemann 
> Cc: Steven Rostedt 
> Cc: Ben Segall 
> Cc: Mel Gorman 
> Cc: Thomas Gleixner 
> Cc: Andy Lutomirski 
> Cc: Will Deacon 
> Cc: "Paul E. McKenney" 
> Cc: Frederic Weisbecker 
> Cc: Allen Pais 
> Cc: Romain Perier 
> To: linux-kernel@vger.kernel.org
> ---
>  include/linux/hardirq.h |  4 ++--
>  include/linux/vtime.h   | 12 ++--
>  kernel/sched/cputime.c  |  4 ++--
>  kernel/softirq.c|  6 +++---
>  4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> index 754f67a..56e7bb5 100644
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -32,7 +32,7 @@ static __always_inline void rcu_irq_enter_check_tick(void)
>   */
>  #define __irq_enter()  \
> do {\
> -   account_irq_enter_time(current);\
> +   account_irq_enter_time(current, true);  \
> preempt_count_add(HARDIRQ_OFFSET);  \
> lockdep_hardirq_enter();\
> } while (0)
> @@ -63,7 +63,7 @@ void irq_enter_rcu(void);
>  #define __irq_exit()   \
> do {\
> lockdep_hardirq_exit(); \
> -   account_irq_exit_time(current); \
> +   account_irq_exit_time(current, true);   \
> preempt_count_sub(HARDIRQ_OFFSET);  \
> } while (0)
>
> diff --git a/include/linux/vtime.h b/include/linux/vtime.h
> index 2cdeca0..294188ae1 100644
> --- a/include/linux/vtime.h
> +++ b/include/linux/vtime.h
> @@ -98,21 +98,21 @@ static inline void vtime_flush(struct task_struct *tsk) { 
> }
>
>
>  #ifdef CONFIG_IRQ_TIME_ACCOUNTING
> -extern void irqtime_account_irq(struct task_struct *tsk);
> +extern void irqtime_account_irq(struct task_struct *tsk, bool hardirq);
>  #else
> -static inline void irqtime_account_irq(struct task_struct *tsk) { }
> +static inline void irqtime_account_irq(struct task_struct *tsk, bool 
> hardirq) { }
>  #endif
>
> -static inline void account_irq_enter_time(struct task_struct *tsk)
> +static inline void account_irq_enter_time(struct task_struct *tsk, bool 
> hardirq)
>  {
> vtime_account_irq_enter(tsk);
> -   irqtime_account_irq(tsk);
> +   irqtime_account_irq(tsk, hardirq);
>  }
>
> -static inline void account_irq_exit_time(struct task_struct *tsk)
> +static inline void account_irq_exit_time(struct task_struct *tsk, bool 
> hardirq)
>  {
> vtime_account_irq_exit(tsk);
> -   irqtime_account_irq(tsk);
> +   irqtime_account_irq(tsk, hardirq);
>  }
>
>  #endif /* _LINUX_KERNEL_VTIME_H */
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 5a55d23..166f1d7 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -47,7 +47,7 @@ static void irqtime_account_delta(struct irqtime *irqtime, 
> u64 delta,
>   * Called before incrementing preempt_count on {soft,}irq_enter
>   * and before decrementing preempt_count on {soft,}irq_exit.
>   */
> -void irqtime_account_irq(struct task_struct *curr)
> +void irqtime_account_irq(struct task_struct *curr, bool hardirq)
>  {
> struct irqtime *irqtime = this_cpu_ptr(_irqtime);
> s64 delta;
> @@ -68,7 +68,7 @@ void irqtime_account_irq(struct task_struct *curr)
>  */
> if (hardirq_count())
> irqtime_account_delta(irqtime, delta, CPUTIME_IRQ);
> -   else if (in_serving_softirq() && curr != this_cpu_ksoftirqd())
> +   else if (in_serving_softirq() && curr != this_cpu_ksoftirqd() && 
> !hardirq)
> irqtime_account_delta(irqtime, delta, CPUTIME_SOFTIRQ);
>  }

In my opinion, we don't need to use the hardirq flag, the code: if
(hardirq_count())
already tell us that where the delt time is from.

Thanks

>  EXPORT_SYMBOL_GPL(irqtime_account_irq);
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index bf88d7f6..da59ea39 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -270,7 +270,7 @@ asmlinkage __visible void __softirq_entry 
> __do_softirq(void)
> current->flags &= ~PF_MEMALLOC;
>
> pending = local_softirq_pending();
> -   account_irq_enter_time(current);
> +   account_irq_enter_time(current, false);
>
> __local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);
> in_hardirq = lockdep_softirq_start();
> @@ -321,7 +321,7 @@ asmlinkage __visible void 

Re: [PATCH 1/1] sched/fair: Fix the wrong sched_stat_wait time

[jun qian]

jun qian  于2020年9月30日周三 下午9:08写道：
>
> Peter Zijlstra  于2020年9月30日周三 下午5:57写道：
> >
> > On Wed, Sep 30, 2020 at 05:16:29PM +0800, jun qian wrote:
> > > Peter Zijlstra  于2020年9月30日周三 下午4:20写道：
> > > >
> > > > On Wed, Sep 30, 2020 at 10:47:12AM +0800, qianjun.ker...@gmail.com 
> > > > wrote:
> > > > > From: jun qian 
> > > > >
> > > > > When the sched_schedstat changes from 0 to 1, some sched se maybe
> > > > > already in the runqueue, the se->statistics.wait_start will be 0.
> > > > > So it will let the (rq_of(cfs_rq)) - se->statistics.wait_start)
> > > > > wrong. We need to avoid this scenario.
> > > >
> > > > Is this really the only problem there? Did you do a full audit of that
> > > > schedstat nonsense?
> > > >
> > >
> > > Did you mean that the sched_stat_xxx's xxx_start(sched_stat_sleep
> > > sched_stat_iowait sched_stat_blocked
> > > sched_stat_runtime) may be also depend the schedstat_enabled?
> >
> > Yeah, this runtime schedstat_enabled thing is fairly recent, it used to
> > be an always on/off kinda thing.
> >
> > At the time we figured inconsistencies from dynamically
> > enabling/disabling it were okay, it's just stats after all.
> >
> > But if you now want to 'fix' that, then a full audit might be nice.
> >
> > > I have searched the codes, and found that these sched_stat_xxx's
> > > xxx_start don't depend the schedstat_enabled
> > > except the wait_start.
> >
> > OK, so you did the audit and only found this one issue? That's good
> > Changelog material :-)
> >
>
> I found another problem, when the sched_schedstat changes from 1 to 0,
> the sched se
> which is already in the runqueue, the statistics.wait_start already
> has a value. At this moment
> sched_schedstat changes from 0 to 1 again, the (rq_of(cfs_rq)) -
> se->statistics.wait_start)
> will not be 0 and the wait time will be bigger than the real one. So
> we need to modify the  patch
> to resolve the problem with this scenario.
>

hi Peter

I have sent another patch to  improve the accuracy of sched_stat_wait
statistics.
I have no good idea to solve the another problem what i described up,
but the new
patch can  improve the accuracy.

Thanks

> So I really need a full audit might, ：-)
>
> Thanks
>
> > Thanks!

Re: [PATCH 1/1] sched/fair: Fix the wrong sched_stat_wait time

[jun qian]

Peter Zijlstra  于2020年9月30日周三 下午5:57写道：
>
> On Wed, Sep 30, 2020 at 05:16:29PM +0800, jun qian wrote:
> > Peter Zijlstra  于2020年9月30日周三 下午4:20写道：
> > >
> > > On Wed, Sep 30, 2020 at 10:47:12AM +0800, qianjun.ker...@gmail.com wrote:
> > > > From: jun qian 
> > > >
> > > > When the sched_schedstat changes from 0 to 1, some sched se maybe
> > > > already in the runqueue, the se->statistics.wait_start will be 0.
> > > > So it will let the (rq_of(cfs_rq)) - se->statistics.wait_start)
> > > > wrong. We need to avoid this scenario.
> > >
> > > Is this really the only problem there? Did you do a full audit of that
> > > schedstat nonsense?
> > >
> >
> > Did you mean that the sched_stat_xxx's xxx_start(sched_stat_sleep
> > sched_stat_iowait sched_stat_blocked
> > sched_stat_runtime) may be also depend the schedstat_enabled?
>
> Yeah, this runtime schedstat_enabled thing is fairly recent, it used to
> be an always on/off kinda thing.
>
> At the time we figured inconsistencies from dynamically
> enabling/disabling it were okay, it's just stats after all.
>
> But if you now want to 'fix' that, then a full audit might be nice.
>
> > I have searched the codes, and found that these sched_stat_xxx's
> > xxx_start don't depend the schedstat_enabled
> > except the wait_start.
>
> OK, so you did the audit and only found this one issue? That's good
> Changelog material :-)
>

I found another problem, when the sched_schedstat changes from 1 to 0,
the sched se
which is already in the runqueue, the statistics.wait_start already
has a value. At this moment
sched_schedstat changes from 0 to 1 again, the (rq_of(cfs_rq)) -
se->statistics.wait_start)
will not be 0 and the wait time will be bigger than the real one. So
we need to modify the  patch
to resolve the problem with this scenario.

So I really need a full audit might, ：-)

Thanks

> Thanks!

Re: [PATCH 1/1] sched/fair: Fix the wrong sched_stat_wait time

[jun qian]

Peter Zijlstra  于2020年9月30日周三 下午4:20写道：
>
> On Wed, Sep 30, 2020 at 10:47:12AM +0800, qianjun.ker...@gmail.com wrote:
> > From: jun qian 
> >
> > When the sched_schedstat changes from 0 to 1, some sched se maybe
> > already in the runqueue, the se->statistics.wait_start will be 0.
> > So it will let the (rq_of(cfs_rq)) - se->statistics.wait_start)
> > wrong. We need to avoid this scenario.
>
> Is this really the only problem there? Did you do a full audit of that
> schedstat nonsense?
>

Did you mean that the sched_stat_xxx's xxx_start(sched_stat_sleep
sched_stat_iowait sched_stat_blocked
sched_stat_runtime) may be also depend the schedstat_enabled?
I have searched the codes, and found that these sched_stat_xxx's
xxx_start don't depend the schedstat_enabled
except the wait_start.

This patch is going to slove the problem that when the
schedstat_enabled is enabled, the sched_stat_wait of
the probed process will become unbelievable big probability in the fist time.

> > Signed-off-by: jun qian 
> > Signed-off-by: Yafang Shao 
> > ---
> >  kernel/sched/fair.c | 8 
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 658aa7a..dd7c3bb 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -908,6 +908,14 @@ static void update_curr_fair(struct rq *rq)
>
> your git-diff is 'funny', it got the function ^ wrong.
>

sorry  :)

> >   if (!schedstat_enabled())
> >   return;
> >
> > + /*
> > +  * When the sched_schedstat changes from 0 to 1, some sched se maybe
> > +  * already in the runqueue, the se->statistics.wait_start will be 0.
> > +  * So it will let the delta wrong. We need to avoid this scenario.
> > +  */
> > + if (unlikely(!schedstat_val(se->statistics.wait_start)))
> > + return;
> > +
> >   delta = rq_clock(rq_of(cfs_rq)) - 
> > schedstat_val(se->statistics.wait_start);
> >
> >   if (entity_is_task(se)) {
> > --
> > 1.8.3.1
> >

Re: [PATCH V7 4/4] softirq: Allow early break the softirq processing loop

[jun qian]

Peter Zijlstra  于2020年9月28日周一 下午5:20写道：
>
> On Tue, Sep 15, 2020 at 07:56:09PM +0800, qianjun.ker...@gmail.com wrote:
> > From: jun qian 
> >
> > Allow terminating the softirq processing loop without finishing the vectors.
> >
> > Signed-off-by: jun qian 
> > ---
> >  kernel/softirq.c | 113 
> > ---
> >  1 file changed, 91 insertions(+), 22 deletions(-)
>
> This is still a ginormous patch for something that should be simple.
>

Yes, i think so. Because of the left pending bits need to be processed,
the code is going to be not simple.I will try my best to simplify this process.

> I've put my patches (and a bunch on top) in:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git core/softirq
>
> but got a fairly significant regression report from 0day on that, and
> haven't had time/motivation to look at that.
>

OK, I am trying to slove the significant regression problem, maybe it
needs some time.

thanks

>

Re: [PATCH V7 4/4] softirq: Allow early break the softirq processing loop

[jun qian]

Frederic Weisbecker  于2020年9月25日周五 上午8:42写道：
>
> On Thu, Sep 24, 2020 at 05:37:42PM +0200, Thomas Gleixner wrote:
> > Subject: softirq; Prevent starvation of higher softirq vectors
> [...]
> > + /*
> > +  * Word swap pending to move the not yet handled bits of the previous
> > +  * run first and then clear the duplicates in the newly raised ones.
> > +  */
> > + swahw32s(_pending);
> > + pending = cur_pending & ~(cur_pending << SIRQ_PREV_SHIFT);
> > +
> >   for_each_set_bit(vec_nr, , NR_SOFTIRQS) {
> >   int prev_count;
> >
> > + vec_nr &= SIRQ_VECTOR_MASK;
>
> Shouldn't NR_SOFTIRQS above protect from that?
>
> >   __clear_bit(vec_nr, );
> >   kstat_incr_softirqs_this_cpu(vec_nr);
> >
> [...]
> > + } else {
> > + /*
> > +  * Retain the unprocessed bits and swap @cur_pending back
> > +  * into normal ordering
> > +  */
> > + cur_pending = (u32)pending;
> > + swahw32s(_pending);
> > + /*
> > +  * If the previous bits are done move the low word of
> > +  * @pending into the high word so it's processed first.
> > +  */
> > + if (!(cur_pending & SIRQ_PREV_MASK))
> > + cur_pending <<= SIRQ_PREV_SHIFT;
>
> If the previous bits are done and there is no timeout, should
> we consider to restart a loop?
>
> A common case would be to enter do_softirq() with RCU_SOFTIRQ set
> in the SIRQ_PREV_MASK and NET_RX_SOFTIRQ set in the normal mask.
>
> You would always end up processing the RCU_SOFTIRQ here and trigger
> ksoftirqd for the NET_RX_SOFTIRQ.

yes, I found that this problem also exists in our project. The RCU
softirq may cost
9ms, that will delay the net_rx/tx softirq to process, Peter's branch
maybe can slove
the problem
git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git core/softirq

>
> Although that's probably no big deal as we should be already in ksoftirqd
> if we processed prev bits. We are just going to iterate the kthread loop
> instead of the do_softirq loop. Probably no real issue then...
>
>
> >
> > + /* Merge the newly pending ones into the low word */
> > + cur_pending |= new_pending;
> > + }
> > + set_softirq_pending(cur_pending);
> >   wakeup_softirqd();
> >  out:
> >   lockdep_softirq_end(in_hardirq);
>

Re: [PATCH V7 4/4] softirq: Allow early break the softirq processing loop

[jun qian]

Thomas Gleixner  于2020年9月24日周四 下午11:37写道：
>
> 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 = sched_clock();
> > @@ -261,8 +277,11 @@ asmlinkage __visible void __softirq_entry 
> > __do_softirq(void)
> >   unsigned int max_restart = 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 for 
> > the
> > @@ -283,25 +302,66 @@ asmlinkage __visible void __softirq_entry 
> > __do_softirq(void)
> >
> >   local_irq_enable();
> >
> > - for_each_set_bit(vec_nr, , NR_SOFTIRQS) {
> > - int prev_count;
> > -
> > - __clear_bit(vec_nr, );
> > -
> > - h = softirq_vec + vec_nr;
> > -
> > - prev_count = 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 handled
> > +  * first in the next time. pending_again means that the new bits is
> > +  * generated in the other time. These bits should be handled after
> > +  * the pending_left bits have been handled.
> > +  *
> > +  * For example
> > +  * If the pending bits is 1101010110, and the loop is broken after
> > +  * the bit4 is handled. Then, the pending_next_bit will be 5, and
> > +  * the pending_left is 110100, the pending_again is 00110.
> > +  */
>
> If you need such a comment to explain the meaning of your variables then
> you did something fundamentaly wrong.
>
> > + next_bit = __this_cpu_read(pending_next_bit);
> > + pending_left = pending &
> > + (SOFTIRQ_PENDING_MASK << next_bit);
> > + pending_again = pending &
> > + (SOFTIRQ_PENDING_MASK >> (NR_SOFTIRQS - next_bit));
> > +
> > + while (pending_left || pending_again) {
> > + if  (pending_left) {
> > + pending = pending_left;
> > + pending_left = 0;
> > + } else if (pending_again) {
> > + pending = pending_again;
> > + pending_again = 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 = 0x25;
> next = 0;
>
> for (...)
> break after bit 0
>
> ==> pending == 0x24
>
> ==> next = 2
>
> now on the next invocation
>
>pending = 0x35;
>next = 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

Re: [PATCH V4] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

Thomas Gleixner  于2020年7月27日周一 下午11:41写道：
>
> Qian,
>
> qianjun.ker...@gmail.com writes:
> >  /*
> >   * We restart softirq processing for at most MAX_SOFTIRQ_RESTART times,
> >   * but break the loop if need_resched() is set or after 2 ms.
> > - * The MAX_SOFTIRQ_TIME provides a nice upper bound in most cases, but in
> > - * certain cases, such as stop_machine(), jiffies may cease to
> > - * increment and so we need the MAX_SOFTIRQ_RESTART limit as
> > - * well to make sure we eventually return from this method.
> > + * In the loop, if the processing time of the softirq has exceeded 2
> > + * milliseconds, we also need to break the loop to wakeup the
> > ksofirqd.
>
> You are removing the MAX_SOFTIRQ_RESTART limit explanation and I rather
> have MAX_SOFTIRQ_TIME_NS there than '2 milliseconds' in case the value
> gets adjusted later on. Also while sched_clock() is granular on many
> systems it still can be jiffies based and then the above problem
> persists.
>
> > @@ -299,6 +298,19 @@ asmlinkage __visible void __softirq_entry 
> > __do_softirq(void)
> >   }
> >   h++;
> >   pending >>= softirq_bit;
> > +
> > + /*
> > +  * the softirq's action has been running for too much time
> > +  * so it may need to wakeup the ksoftirqd
> > +  */
> > + if (need_resched() && sched_clock() > end) {
> > + /*
> > +  * Ensure that the remaining pending bits are
> > +  * handled.
> > +  */
> > + or_softirq_pending(pending << (vec_nr + 1));
>
> To or the value interrupts need to be disabled because otherwise you can
> lose a bit when an interrupt happens in the middle of the RMW operation
> and raises a softirq which is not in @pending and not in the per CPU
> local softirq pending storage.
>
> There is another problem. 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 this needs more thought.
HI Thomas, The problem as you described, i am trying to solve it, but
i found the
other problem,just like this,for example

the pending bits is 1010110110, when the bit4 was handled, and the
loop processing
time more than 2ms, in my patch, the loop will break early. In the
next time, the loop
will process the bit5, if before the soft action, the
bit6,bit8,bit0,bit3 were set, the loop
will process the bit5,6,7,8,9,0,1,2,3, that will be the bit6 and bit8
wil be processed before
bit0 and bit3.

Do we need to consider this scenario. Do you have any good suggestions.

Thanks.
>
> Thanks,
>
> tglx

Re: [PATCH V6 1/1] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

 于2020年9月11日周五 下午11:55写道：
>
> On Wed, Sep 09, 2020 at 05:09:31PM +0800, qianjun.ker...@gmail.com wrote:
> > From: jun qian 
> >
> > 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 200
>
> 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.
> > +  */
&g

Re: [PATCH V4] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

Thomas Gleixner  于2020年7月29日周三 下午8:16写道：
>
> Qian,
>
> jun qian  writes:
> > On Mon, Jul 27, 2020 at 11:41 PM Thomas Gleixner  wrote:
> >> > + or_softirq_pending(pending << (vec_nr + 1));
> >>
> >> To or the value interrupts need to be disabled because otherwise you can
> >> lose a bit when an interrupt happens in the middle of the RMW operation
> >> and raises a softirq which is not in @pending and not in the per CPU
> >> local softirq pending storage.
> >>
> > I can't understand the problem described above, could you explain in
> > detail.
>
> Oring a value to a memory location is a Read Modify Write (RMW)
> operation.
>
> x |= a;
>
> translates to pseudo code:
>
> R1 =  x  // Load content of memory location x into register R1
> R1 |= a  // Or value a on R1
> x = R1   // Write back result
>
> So assume:
>
>x = 0
>a = 1
>
>R1 = x  --> R1 == 0
>R1 |= a --> R1 == 1
>
> interrupt sets bit 1 in x   --> x == 0x02
>
>x = R1  --> x == 1
>
> So you lost the set bit 1, right? Not really what you want.
>
> >> There is another problem. 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.
> >>
> > May I use a percpu val to record the order of processing softirq, by the 
> > order
> > val, when it is in ksoftirqd we can process the pending softirq in order. 
> > In the
> > scenario you described above, before ksoftirqd runs, bit 0 gets raised 
> > again,
> > ksoftirqd runs and handles bit 1 by the percpu val. just like a ring.
>
> Yes, you need something to save information about the not-processed
> bits. Keeping track of which bit to process next works, but whether
> that's going to result in efficient and simple code is a different
> question.
>
> Thanks,
>
> tglx
>

Hi  Thomas, I am so sorry,   For personal reasons, the modification of
this patch was delayed, I will submit the next modified version in
these two days, sorry again

Re: [PATCH V4] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

On Wed, Jul 29, 2020 at 8:16 PM Thomas Gleixner  wrote:
>
> Qian,
>
> jun qian  writes:
> > On Mon, Jul 27, 2020 at 11:41 PM Thomas Gleixner  wrote:
> >> > + or_softirq_pending(pending << (vec_nr + 1));
> >>
> >> To or the value interrupts need to be disabled because otherwise you can
> >> lose a bit when an interrupt happens in the middle of the RMW operation
> >> and raises a softirq which is not in @pending and not in the per CPU
> >> local softirq pending storage.
> >>
> > I can't understand the problem described above, could you explain in
> > detail.
>
> Oring a value to a memory location is a Read Modify Write (RMW)
> operation.
>
> x |= a;
>
> translates to pseudo code:
>
> R1 =  x  // Load content of memory location x into register R1
> R1 |= a  // Or value a on R1
> x = R1   // Write back result
>
> So assume:
>
>x = 0
>a = 1
>
>R1 = x  --> R1 == 0
>R1 |= a --> R1 == 1
>
> interrupt sets bit 1 in x   --> x == 0x02
>
>x = R1  --> x == 1
>
> So you lost the set bit 1, right? Not really what you want.
>
wow, thanks a lot, i got it.

> >> There is another problem. 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.
> >>
> > May I use a percpu val to record the order of processing softirq, by the 
> > order
> > val, when it is in ksoftirqd we can process the pending softirq in order. 
> > In the
> > scenario you described above, before ksoftirqd runs, bit 0 gets raised 
> > again,
> > ksoftirqd runs and handles bit 1 by the percpu val. just like a ring.
>
> Yes, you need something to save information about the not-processed
> bits. Keeping track of which bit to process next works, but whether
> that's going to result in efficient and simple code is a different
> question.
>
ok, i will modify it in the next version.

> Thanks,
>
> tglx
>

Re: [PATCH V4] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

On Mon, Jul 27, 2020 at 11:41 PM Thomas Gleixner  wrote:
>
> Qian,
>
> qianjun.ker...@gmail.com writes:
> >  /*
> >   * We restart softirq processing for at most MAX_SOFTIRQ_RESTART times,
> >   * but break the loop if need_resched() is set or after 2 ms.
> > - * The MAX_SOFTIRQ_TIME provides a nice upper bound in most cases, but in
> > - * certain cases, such as stop_machine(), jiffies may cease to
> > - * increment and so we need the MAX_SOFTIRQ_RESTART limit as
> > - * well to make sure we eventually return from this method.
> > + * In the loop, if the processing time of the softirq has exceeded 2
> > + * milliseconds, we also need to break the loop to wakeup the
> > ksofirqd.
>
> You are removing the MAX_SOFTIRQ_RESTART limit explanation and I rather
> have MAX_SOFTIRQ_TIME_NS there than '2 milliseconds' in case the value
> gets adjusted later on. Also while sched_clock() is granular on many
> systems it still can be jiffies based and then the above problem
> persists.
>
> > @@ -299,6 +298,19 @@ asmlinkage __visible void __softirq_entry 
> > __do_softirq(void)
> >   }
> >   h++;
> >   pending >>= softirq_bit;
> > +
> > + /*
> > +  * the softirq's action has been running for too much time
> > +  * so it may need to wakeup the ksoftirqd
> > +  */
> > + if (need_resched() && sched_clock() > end) {
> > + /*
> > +  * Ensure that the remaining pending bits are
> > +  * handled.
> > +  */
> > + or_softirq_pending(pending << (vec_nr + 1));
>
> To or the value interrupts need to be disabled because otherwise you can
> lose a bit when an interrupt happens in the middle of the RMW operation
> and raises a softirq which is not in @pending and not in the per CPU
> local softirq pending storage.
>
I can't understand the problem described above, could you explain in detail.

thanks.

> There is another problem. 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.
>
May I use a percpu val to record the order of processing softirq, by the order
val, when it is in ksoftirqd we can process the pending softirq in order. In the
scenario you described above, before ksoftirqd runs, bit 0 gets raised again,
ksoftirqd runs and handles bit 1 by the percpu val. just like a ring.

Looking forward to your suggestions

Thanks
> So this needs more thought.
>
> Thanks,
>
> tglx

Re: [PATCH V4] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

On Mon, Jul 27, 2020 at 11:41 PM Thomas Gleixner  wrote:
>
> Qian,
>
> qianjun.ker...@gmail.com writes:
> >  /*
> >   * We restart softirq processing for at most MAX_SOFTIRQ_RESTART times,
> >   * but break the loop if need_resched() is set or after 2 ms.
> > - * The MAX_SOFTIRQ_TIME provides a nice upper bound in most cases, but in
> > - * certain cases, such as stop_machine(), jiffies may cease to
> > - * increment and so we need the MAX_SOFTIRQ_RESTART limit as
> > - * well to make sure we eventually return from this method.
> > + * In the loop, if the processing time of the softirq has exceeded 2
> > + * milliseconds, we also need to break the loop to wakeup the
> > ksofirqd.
>
> You are removing the MAX_SOFTIRQ_RESTART limit explanation and I rather
> have MAX_SOFTIRQ_TIME_NS there than '2 milliseconds' in case the value
> gets adjusted later on. Also while sched_clock() is granular on many
> systems it still can be jiffies based and then the above problem
> persists.
>
> > @@ -299,6 +298,19 @@ asmlinkage __visible void __softirq_entry 
> > __do_softirq(void)
> >   }
> >   h++;
> >   pending >>= softirq_bit;
> > +
> > + /*
> > +  * the softirq's action has been running for too much time
> > +  * so it may need to wakeup the ksoftirqd
> > +  */
> > + if (need_resched() && sched_clock() > end) {
> > + /*
> > +  * Ensure that the remaining pending bits are
> > +  * handled.
> > +  */
> > + or_softirq_pending(pending << (vec_nr + 1));
>
> To or the value interrupts need to be disabled because otherwise you can
> lose a bit when an interrupt happens in the middle of the RMW operation
> and raises a softirq which is not in @pending and not in the per CPU
> local softirq pending storage.
>
> There is another problem. 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.
>
I got it. I will try to slove this problem. Thanks.

> So this needs more thought.
>
> Thanks,
>
> tglx

Re: [Softirq] a76eadba0d: WARNING:at_net/mac80211/rx.c:#ieee80211_rx_napi[mac80211]

[jun qian]

On Thu, Jul 23, 2020 at 10:35 PM Thomas Gleixner  wrote:
>
> jun qian  writes:
> > On Thu, Jul 23, 2020 at 6:58 PM Thomas Gleixner  wrote:
> >> That drops everything which has not yet been processed and the above
> >> warning is due to this.
> >>
> > wow, I made a mistake, thank you for finding the cause of the problem
> > so quickly.
> >
> > How about the following code.   we need to clear the corresponding
> > pending bit at the
> > right time Instead of all the pending bits cleared in the start.
> >
> > pending = softirq_pending();
> >
> > while ((softirq_bit = ffs(pending))) {
> >
> > pending >>= softirq_bit;
> >
> > set_softirq_pending(pending);  //Only clear the corresponding
> > bit which will be processed.
>
> How is that supposed to be correct. pending has been shifted
> right. Something like this should work:
>
> h++;
> pending >>= softirq_bit;
>
> if (timeout()) {
> /*
>  * Ensure that the remaining pending bits
>  * are handled.
>  */
> or_softirq_pending(pending << (vec_nr + 1));
> break;
> }
> }
>
> Thanks,
>
> tglx
>

I have two questions that need to be discussed.

1. If the __do_sofrirq() is executed in the ksoftirqd, we may not need
to check the timeout in the loop.
2. Both the invoke_softirq() and run_ksoftirqd()  will execute
__do_sofirq, they all execute the same code，
when it is in the ksoftirqd, Do we need to wake up ksoftirqd in
the process context according to
max_restart and MAX_SOFTIRQ_TIME. In my opinion, If we  use a flag
to distinguish where
__do_softirq() is called from,  we can do what is most suitable
for __do_softirq based on this flag.

Re: [PATCH V3] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

On Thu, Jul 23, 2020 at 9:41 PM Thomas Gleixner  wrote:
>
> qianjun.ker...@gmail.com writes:
> > From: jun qian 
> > + /*
> > +  * the softirq's action has been running for too much time
> > +  * so it may need to wakeup the ksoftirqd
> > +  */
> > + if (need_resched() && ktime_get() > end)
> > + break;
>
> As per my reply on V2 this is leaking non handled pending bits. If you
> do a V4, can you please use sched_clock() instead of ktime_get()?
>
The reason why the non handled pending bits leaked is
set_softirq_pending(0) called in the start, if the
loop is broken, the not handled bit will leak. This is my
understanding, I am not sure if it is correct or not.
Looking forward to your reply.

Thank you so much.

> Thanks,
>
> tglx

Re: [Softirq] a76eadba0d: WARNING:at_net/mac80211/rx.c:#ieee80211_rx_napi[mac80211]

[jun qian]

On Thu, Jul 23, 2020 at 6:58 PM Thomas Gleixner  wrote:
>
> kernel test robot  writes:
> > [  106.856151] WARNING: CPU: 5 PID: 4569 at net/mac80211/rx.c:4708 
> > ieee80211_rx_napi+0x44d/0x560 [mac80211]
>
> Bah. I clearly should have noticed when looking at the patch.
>
>  pending = softirq_pending();
>
>  set_softirq_pending(0);
>
>  while (pending) {
>
>
>if (timeout)
> break;
>  }
>
> That drops everything which has not yet been processed and the above
> warning is due to this.
>
wow, I made a mistake, thank you for finding the cause of the problem
so quickly.

How about the following code.   we need to clear the corresponding
pending bit at the
right time Instead of all the pending bits cleared in the start.

pending = softirq_pending();

while ((softirq_bit = ffs(pending))) {

pending >>= softirq_bit;

set_softirq_pending(pending);  //Only clear the corresponding
bit which will be processed.

h->action(h);

if (timeout)
  break;
}

> Thanks,
>
> tglx
>

Re: [RFC PATCH v2] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

On Thu, Jul 23, 2020 at 2:05 AM Thomas Gleixner  wrote:
>
> qianjun.ker...@gmail.com writes:
> > +
> > + end = ktime_get();
> > + delta = ktime_to_us(end - start);
>
> What's the point of this conversion? That's a division for no value
> because you can simply define the maximum time in nanoseconds with the
> same effect, i.e.
>
> ktime_t end = ktime_get() + MAX_SOFTIRQ_TIME_NS;
>
> if (need_resched() && ktime_get() > end)
> break;
>
> So you can spare all that start, delta and conversion dance and keep the
> code simple.
>
> Also notice that need_resched() wants to be evaluated first because
> there is no point to do the more expensive time read if need_resched()
> is false.
good suggestion，Thanks

I will make changes in the next version
>
> Thanks,
>
> tglx

Re: [PATCH] perf-c2c: Fix the wrong description.

[jun qian]

On Thu, Jul 9, 2020 at 9:07 PM qianjun  wrote:
>
> From: qianjun 
>
> Use L1Miss to replace L1Hit to describe the correct scene
>
> Signed-off-by: qianjun 
> ---
>  tools/perf/Documentation/perf-c2c.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/Documentation/perf-c2c.txt 
> b/tools/perf/Documentation/perf-c2c.txt
> index 98efdab..083e99a 100644
> --- a/tools/perf/Documentation/perf-c2c.txt
> +++ b/tools/perf/Documentation/perf-c2c.txt
> @@ -186,7 +186,7 @@ For each cacheline in the 1) list we display following 
> data:
>Store Reference - Total, L1Hit, L1Miss
>  Total - all store accesses
>  L1Hit - store accesses that hit L1
> -L1Hit - store accesses that missed L1
> +L1Miss - store accesses that missed L1
>
>Load Dram
>- count of local and remote DRAM accesses
> --
> 1.8.3.1
>

hi man

I think it's a problem :)

Re: [RFC PATCH v2] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

Hi Peter & Uladzislau

Are there any issues that have not been considered in this patch? Can
you give me some suggestions on this issue. If the situation I
described is indeed a problem，how about this modification. Thanks a
lot.

On Mon, Jul 20, 2020 at 9:09 PM  wrote:
>
> From: jun qian 
>
> 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.
>
> Signed-off-by: jun qian 
> ---
>  kernel/softirq.c | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index c4201b7f..f8e5be9 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -210,7 +210,7 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int 
> cnt)
>   * we want to handle softirqs as soon as possible, but they
>   * should not be able to lock up the box.
>   */
> -#define MAX_SOFTIRQ_TIME  msecs_to_jiffies(2)
> +#define MAX_SOFTIRQ_TIME 2000  /* In microseconds */
>  #define MAX_SOFTIRQ_RESTART 10
>
>  #ifdef CONFIG_TRACE_IRQFLAGS
> @@ -248,7 +248,8 @@ static inline void lockdep_softirq_end(bool in_hardirq) { 
> }
>
>  asmlinkage __visible void __softirq_entry __do_softirq(void)
>  {
> -   unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
> +   ktime_t end, start;
> +   s64 delta;
> unsigned long old_flags = current->flags;
> int max_restart = MAX_SOFTIRQ_RESTART;
> struct softirq_action *h;
> @@ -256,6 +257,8 @@ asmlinkage __visible void __softirq_entry 
> __do_softirq(void)
> __u32 pending;
> int softirq_bit;
>
> +   start = ktime_get();
> +
> /*
>  * Mask out PF_MEMALLOC as the current task context is borrowed for 
> the
>  * softirq. A softirq handled, such as network RX, might set 
> PF_MEMALLOC
> @@ -299,6 +302,15 @@ asmlinkage __visible void __softirq_entry 
> __do_softirq(void)
> }
> h++;
> pending >>= softirq_bit;
> +
> +   end = ktime_get();
> +   delta = ktime_to_us(end - start);
> +   /*
> +* the softirq's action has been running for too much time
> +* so it may need to wakeup the ksoftirqd
> +*/
> +   if (delta > MAX_SOFTIRQ_TIME && need_resched())
> +   break;
> }
>
> if (__this_cpu_read(ksoftirqd) == current)
> @@ -307,7 +319,9 @@ asmlinkage __visible void __softirq_entry 
> __do_softirq(void)
>
> pending = local_softirq_pending();
> if (pending) {
> -   if (time_before(jiffies, end) && !need_resched() &&
> +   end = ktime_get();
> +   delta = ktime_to_us(end - start);
> +   if (delta < MAX_SOFTIRQ_TIME && !need_resched() &&
> --max_restart)
> goto restart;
>
> --
> 1.8.3.1
>

Re: [RFC PATCH 1/1] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

On Sat, Jul 18, 2020 at 6:07 AM Uladzislau Rezki  wrote:
>
> > From: jun qian 
> >
> > 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.
> >
> > Signed-off-by: jun qian 
> > ---
> >  kernel/softirq.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > index c4201b7f..602d9fa 100644
> > --- a/kernel/softirq.c
> > +++ b/kernel/softirq.c
> > @@ -299,6 +299,9 @@ asmlinkage __visible void __softirq_entry 
> > __do_softirq(void)
> >   }
> >   h++;
> >   pending >>= softirq_bit;
> > +
> > + if (time_after(jiffies, end) && need_resched())
> > + break;
> >   }
> >
> >   if (__this_cpu_read(ksoftirqd) == current)
> >
> I have a small concern about MAX_SOFTIRQ_TIME. The problem is that
> an "end" time is based on jiffies/tick update, so it depends on CONFIG_HZ
> value of your kernel.
>
> For example if we have CONFIG_HZ=100, msecs_to_jiffies(2) will return 1.
> For HZ=100 one jiffie is 10 milliseconds. So we can not rely on it,
> because of low resolution.
>
 good tip. Does this problem also exist in the current code, just like this:

if (pending) {
if (time_before(jiffies, end) && !need_resched() &&
/* low resolution problem */
--max_restart)
goto restart;

wakeup_softirqd();
}

> Maybe it make sense to fix it first in order to be at least aligned with
> "2 milliseconds time limit" documentation?
>
> 
>  * We restart softirq processing for at most MAX_SOFTIRQ_RESTART times,
>  * but break the loop if need_resched() is set or after 2 ms.
> 
>
I can't find the snip from the linux/Documentation/, could you please
tell me where I can find this snip, thks

> ktime_get()/ktime_before()...?
>
if the low resolution problem also exists in the above code, i think
also need to fix it with using ktime_get()/ktime_before().

> --
> Vlad Rezki

[PATCH] mm:slab: Adjust the print format for the slabinfo

[jun qian]

Header and the corresponding information is not aligned,
adjust the printing format helps us to understand the slabinfo better.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 mm/slab_common.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index fea3376f9816..07a324cbbfb6 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -1263,9 +1263,13 @@ static void print_slabinfo_header(struct seq_file *m)
 #else
seq_puts(m, "slabinfo - version: 2.1\n");
 #endif
-   seq_puts(m, "# name   
 ");
-   seq_puts(m, " : tunables   ");
-   seq_puts(m, " : slabdata   ");
+   seq_printf(m, "%-22s %-14s %-11s %-10s %-13s %-14s",
+ "# name", "", "", "",
+ "", "");
+   seq_printf(m, " : %-9s %-8s %-13s %-14s",
+ "tunables", "", "", "");
+   seq_printf(m, " : %-9s %-15s %-12s %-16s",
+ "slabdata", "", "", "");
 #ifdef CONFIG_DEBUG_SLAB
seq_puts(m, " : globalstat 
");
seq_puts(m, " : cpustat");
@@ -1319,13 +1323,13 @@ static void cache_show(struct kmem_cache *s, struct 
seq_file *m)
 
memcg_accumulate_slabinfo(s, );
 
-   seq_printf(m, "%-17s %6lu %6lu %6u %4u %4d",
+   seq_printf(m, "%-22s %-14lu %-11lu %-10u %-13u %-14d",
   cache_name(s), sinfo.active_objs, sinfo.num_objs, s->size,
   sinfo.objects_per_slab, (1 << sinfo.cache_order));
 
-   seq_printf(m, " : tunables %4u %4u %4u",
+   seq_printf(m, " : %-9s %-8u %-13u %-14u", "tunables",
   sinfo.limit, sinfo.batchcount, sinfo.shared);
-   seq_printf(m, " : slabdata %6lu %6lu %6lu",
+   seq_printf(m, " : %-9s %-15lu %-12lu %-16lu", "slabdata",
   sinfo.active_slabs, sinfo.num_slabs, sinfo.shared_avail);
slabinfo_show_stats(m, s);
seq_putc(m, '\n');
-- 
2.17.1

[PATCH] mm:slab: Adjust the print format for the slabinfo

[jun qian]

Header and the corresponding information is not aligned,
adjust the printing format helps us to understand the slabinfo better.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 mm/slab_common.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index fea3376f9816..07a324cbbfb6 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -1263,9 +1263,13 @@ static void print_slabinfo_header(struct seq_file *m)
 #else
seq_puts(m, "slabinfo - version: 2.1\n");
 #endif
-   seq_puts(m, "# name   
 ");
-   seq_puts(m, " : tunables   ");
-   seq_puts(m, " : slabdata   ");
+   seq_printf(m, "%-22s %-14s %-11s %-10s %-13s %-14s",
+ "# name", "", "", "",
+ "", "");
+   seq_printf(m, " : %-9s %-8s %-13s %-14s",
+ "tunables", "", "", "");
+   seq_printf(m, " : %-9s %-15s %-12s %-16s",
+ "slabdata", "", "", "");
 #ifdef CONFIG_DEBUG_SLAB
seq_puts(m, " : globalstat 
");
seq_puts(m, " : cpustat");
@@ -1319,13 +1323,13 @@ static void cache_show(struct kmem_cache *s, struct 
seq_file *m)
 
memcg_accumulate_slabinfo(s, );
 
-   seq_printf(m, "%-17s %6lu %6lu %6u %4u %4d",
+   seq_printf(m, "%-22s %-14lu %-11lu %-10u %-13u %-14d",
   cache_name(s), sinfo.active_objs, sinfo.num_objs, s->size,
   sinfo.objects_per_slab, (1 << sinfo.cache_order));
 
-   seq_printf(m, " : tunables %4u %4u %4u",
+   seq_printf(m, " : %-9s %-8u %-13u %-14u", "tunables",
   sinfo.limit, sinfo.batchcount, sinfo.shared);
-   seq_printf(m, " : slabdata %6lu %6lu %6lu",
+   seq_printf(m, " : %-9s %-15lu %-12lu %-16lu", "slabdata",
   sinfo.active_slabs, sinfo.num_slabs, sinfo.shared_avail);
slabinfo_show_stats(m, s);
seq_putc(m, '\n');
-- 
2.17.1

[PATCH] i2c: i2c-tegra: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
---
 drivers/i2c/busses/i2c-tegra.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 60c8561fbe65..59f31d3a508f 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -608,11 +608,10 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
u32 status;
const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
struct tegra_i2c_dev *i2c_dev = dev_id;
-   unsigned long flags;
 
status = i2c_readl(i2c_dev, I2C_INT_STATUS);
 
-   spin_lock_irqsave(_dev->xfer_lock, flags);
+   spin_lock(_dev->xfer_lock);
if (status == 0) {
dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
 i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS),
@@ -670,7 +669,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 
complete(_dev->msg_complete);
 done:
-   spin_unlock_irqrestore(_dev->xfer_lock, flags);
+   spin_unlock(_dev->xfer_lock);
return IRQ_HANDLED;
 }
 
-- 
2.17.1

[PATCH] i2c: i2c-tegra: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
---
 drivers/i2c/busses/i2c-tegra.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 60c8561fbe65..59f31d3a508f 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -608,11 +608,10 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
u32 status;
const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
struct tegra_i2c_dev *i2c_dev = dev_id;
-   unsigned long flags;
 
status = i2c_readl(i2c_dev, I2C_INT_STATUS);
 
-   spin_lock_irqsave(_dev->xfer_lock, flags);
+   spin_lock(_dev->xfer_lock);
if (status == 0) {
dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
 i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS),
@@ -670,7 +669,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 
complete(_dev->msg_complete);
 done:
-   spin_unlock_irqrestore(_dev->xfer_lock, flags);
+   spin_unlock(_dev->xfer_lock);
return IRQ_HANDLED;
 }
 
-- 
2.17.1

[PATCH] mmc: mxcmmc: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
---
 drivers/mmc/host/mxcmmc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index de4e6e5bf304..4d17032d15ee 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -728,7 +728,6 @@ static void mxcmci_cmd_done(struct mxcmci_host *host, 
unsigned int stat)
 static irqreturn_t mxcmci_irq(int irq, void *devid)
 {
struct mxcmci_host *host = devid;
-   unsigned long flags;
bool sdio_irq;
u32 stat;
 
@@ -740,9 +739,9 @@ static irqreturn_t mxcmci_irq(int irq, void *devid)
 
dev_dbg(mmc_dev(host->mmc), "%s: 0x%08x\n", __func__, stat);
 
-   spin_lock_irqsave(>lock, flags);
+   spin_lock(>lock);
sdio_irq = (stat & STATUS_SDIO_INT_ACTIVE) && host->use_sdio;
-   spin_unlock_irqrestore(>lock, flags);
+   spin_unlock(>lock);
 
if (mxcmci_use_dma(host) && (stat & (STATUS_WRITE_OP_DONE)))
mxcmci_writel(host, STATUS_WRITE_OP_DONE, MMC_REG_STATUS);
-- 
2.17.1

[PATCH] mmc: mxcmmc: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
---
 drivers/mmc/host/mxcmmc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index de4e6e5bf304..4d17032d15ee 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -728,7 +728,6 @@ static void mxcmci_cmd_done(struct mxcmci_host *host, 
unsigned int stat)
 static irqreturn_t mxcmci_irq(int irq, void *devid)
 {
struct mxcmci_host *host = devid;
-   unsigned long flags;
bool sdio_irq;
u32 stat;
 
@@ -740,9 +739,9 @@ static irqreturn_t mxcmci_irq(int irq, void *devid)
 
dev_dbg(mmc_dev(host->mmc), "%s: 0x%08x\n", __func__, stat);
 
-   spin_lock_irqsave(>lock, flags);
+   spin_lock(>lock);
sdio_irq = (stat & STATUS_SDIO_INT_ACTIVE) && host->use_sdio;
-   spin_unlock_irqrestore(>lock, flags);
+   spin_unlock(>lock);
 
if (mxcmci_use_dma(host) && (stat & (STATUS_WRITE_OP_DONE)))
mxcmci_writel(host, STATUS_WRITE_OP_DONE, MMC_REG_STATUS);
-- 
2.17.1

[PATCH] dmaengine: idma64: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/dma/idma64.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c
index 1fbf9cb9b742..5b9c1566ad73 100644
--- a/drivers/dma/idma64.c
+++ b/drivers/dma/idma64.c
@@ -142,9 +142,8 @@ static void idma64_chan_irq(struct idma64 *idma64, unsigned 
short c,
 {
struct idma64_chan *idma64c = >chan[c];
struct idma64_desc *desc;
-   unsigned long flags;
 
-   spin_lock_irqsave(>vchan.lock, flags);
+   spin_lock(>vchan.lock);
desc = idma64c->desc;
if (desc) {
if (status_err & (1 << c)) {
@@ -161,7 +160,7 @@ static void idma64_chan_irq(struct idma64 *idma64, unsigned 
short c,
if (idma64c->desc == NULL || desc->status == DMA_ERROR)
idma64_stop_transfer(idma64c);
}
-   spin_unlock_irqrestore(>vchan.lock, flags);
+   spin_unlock(>vchan.lock);
 }
 
 static irqreturn_t idma64_irq(int irq, void *dev)
-- 
2.17.1

[PATCH] dmaengine: idma64: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/dma/idma64.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c
index 1fbf9cb9b742..5b9c1566ad73 100644
--- a/drivers/dma/idma64.c
+++ b/drivers/dma/idma64.c
@@ -142,9 +142,8 @@ static void idma64_chan_irq(struct idma64 *idma64, unsigned 
short c,
 {
struct idma64_chan *idma64c = >chan[c];
struct idma64_desc *desc;
-   unsigned long flags;
 
-   spin_lock_irqsave(>vchan.lock, flags);
+   spin_lock(>vchan.lock);
desc = idma64c->desc;
if (desc) {
if (status_err & (1 << c)) {
@@ -161,7 +160,7 @@ static void idma64_chan_irq(struct idma64 *idma64, unsigned 
short c,
if (idma64c->desc == NULL || desc->status == DMA_ERROR)
idma64_stop_transfer(idma64c);
}
-   spin_unlock_irqrestore(>vchan.lock, flags);
+   spin_unlock(>vchan.lock);
 }
 
 static irqreturn_t idma64_irq(int irq, void *dev)
-- 
2.17.1

[PATCH] dmaengine: zx-dma: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/dma/zx_dma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/zx_dma.c b/drivers/dma/zx_dma.c
index 2571bc7693df..111ab2db83b1 100644
--- a/drivers/dma/zx_dma.c
+++ b/drivers/dma/zx_dma.c
@@ -288,9 +288,8 @@ static irqreturn_t zx_dma_int_handler(int irq, void *dev_id)
p = >phy[i];
c = p->vchan;
if (c) {
-   unsigned long flags;
 
-   spin_lock_irqsave(>vc.lock, flags);
+   spin_lock(>vc.lock);
if (c->cyclic) {
vchan_cyclic_callback(>ds_run->vd);
} else {
@@ -298,7 +297,7 @@ static irqreturn_t zx_dma_int_handler(int irq, void *dev_id)
p->ds_done = p->ds_run;
task = 1;
}
-   spin_unlock_irqrestore(>vc.lock, flags);
+   spin_unlock(>vc.lock);
irq_chan |= BIT(i);
}
}
-- 
2.17.1

[PATCH] dmaengine: zx-dma: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/dma/zx_dma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/zx_dma.c b/drivers/dma/zx_dma.c
index 2571bc7693df..111ab2db83b1 100644
--- a/drivers/dma/zx_dma.c
+++ b/drivers/dma/zx_dma.c
@@ -288,9 +288,8 @@ static irqreturn_t zx_dma_int_handler(int irq, void *dev_id)
p = >phy[i];
c = p->vchan;
if (c) {
-   unsigned long flags;
 
-   spin_lock_irqsave(>vc.lock, flags);
+   spin_lock(>vc.lock);
if (c->cyclic) {
vchan_cyclic_callback(>ds_run->vd);
} else {
@@ -298,7 +297,7 @@ static irqreturn_t zx_dma_int_handler(int irq, void *dev_id)
p->ds_done = p->ds_run;
task = 1;
}
-   spin_unlock_irqrestore(>vc.lock, flags);
+   spin_unlock(>vc.lock);
irq_chan |= BIT(i);
}
}
-- 
2.17.1

[PATCH] dmaengine: k3dma: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/dma/k3dma.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index 6bfa217ed6d0..30052258ffd5 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -216,22 +216,21 @@ static irqreturn_t k3_dma_int_handler(int irq, void 
*dev_id)
i = __ffs(stat);
stat &= ~BIT(i);
if (likely(tc1 & BIT(i)) || (tc2 & BIT(i))) {
-   unsigned long flags;
 
p = >phy[i];
c = p->vchan;
if (c && (tc1 & BIT(i))) {
-   spin_lock_irqsave(>vc.lock, flags);
+   spin_lock(>vc.lock);
vchan_cookie_complete(>ds_run->vd);
p->ds_done = p->ds_run;
p->ds_run = NULL;
-   spin_unlock_irqrestore(>vc.lock, flags);
+   spin_unlock(>vc.lock);
}
if (c && (tc2 & BIT(i))) {
-   spin_lock_irqsave(>vc.lock, flags);
+   spin_lock(>vc.lock);
if (p->ds_run != NULL)
vchan_cyclic_callback(>ds_run->vd);
-   spin_unlock_irqrestore(>vc.lock, flags);
+   spin_unlock(>vc.lock);
}
irq_chan |= BIT(i);
}
-- 
2.17.1

[PATCH] dmaengine: k3dma: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/dma/k3dma.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index 6bfa217ed6d0..30052258ffd5 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -216,22 +216,21 @@ static irqreturn_t k3_dma_int_handler(int irq, void 
*dev_id)
i = __ffs(stat);
stat &= ~BIT(i);
if (likely(tc1 & BIT(i)) || (tc2 & BIT(i))) {
-   unsigned long flags;
 
p = >phy[i];
c = p->vchan;
if (c && (tc1 & BIT(i))) {
-   spin_lock_irqsave(>vc.lock, flags);
+   spin_lock(>vc.lock);
vchan_cookie_complete(>ds_run->vd);
p->ds_done = p->ds_run;
p->ds_run = NULL;
-   spin_unlock_irqrestore(>vc.lock, flags);
+   spin_unlock(>vc.lock);
}
if (c && (tc2 & BIT(i))) {
-   spin_lock_irqsave(>vc.lock, flags);
+   spin_lock(>vc.lock);
if (p->ds_run != NULL)
vchan_cyclic_callback(>ds_run->vd);
-   spin_unlock_irqrestore(>vc.lock, flags);
+   spin_unlock(>vc.lock);
}
irq_chan |= BIT(i);
}
-- 
2.17.1

[PATCH] dmaengine: moxart-dma: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/dma/moxart-dma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/moxart-dma.c b/drivers/dma/moxart-dma.c
index e04499c1f27f..77958f123226 100644
--- a/drivers/dma/moxart-dma.c
+++ b/drivers/dma/moxart-dma.c
@@ -524,7 +524,6 @@ static irqreturn_t moxart_dma_interrupt(int irq, void 
*devid)
struct moxart_dmadev *mc = devid;
struct moxart_chan *ch = >slave_chans[0];
unsigned int i;
-   unsigned long flags;
u32 ctrl;
 
dev_dbg(chan2dev(>vc.chan), "%s\n", __func__);
@@ -541,14 +540,14 @@ static irqreturn_t moxart_dma_interrupt(int irq, void 
*devid)
if (ctrl & APB_DMA_FIN_INT_STS) {
ctrl &= ~APB_DMA_FIN_INT_STS;
if (ch->desc) {
-   spin_lock_irqsave(>vc.lock, flags);
+   spin_lock(>vc.lock);
if (++ch->sgidx < ch->desc->sglen) {
moxart_dma_start_sg(ch, ch->sgidx);
} else {
vchan_cookie_complete(>desc->vd);
moxart_dma_start_desc(>vc.chan);
}
-   spin_unlock_irqrestore(>vc.lock, flags);
+   spin_unlock(>vc.lock);
}
}
 
-- 
2.17.1

[PATCH] dmaengine: moxart-dma: replace spin_lock_irqsave with spin_lock in ISR

[jun qian]

As you are already in ISR, it is unnecessary to call spin_lock_irqsave.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/dma/moxart-dma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/moxart-dma.c b/drivers/dma/moxart-dma.c
index e04499c1f27f..77958f123226 100644
--- a/drivers/dma/moxart-dma.c
+++ b/drivers/dma/moxart-dma.c
@@ -524,7 +524,6 @@ static irqreturn_t moxart_dma_interrupt(int irq, void 
*devid)
struct moxart_dmadev *mc = devid;
struct moxart_chan *ch = >slave_chans[0];
unsigned int i;
-   unsigned long flags;
u32 ctrl;
 
dev_dbg(chan2dev(>vc.chan), "%s\n", __func__);
@@ -541,14 +540,14 @@ static irqreturn_t moxart_dma_interrupt(int irq, void 
*devid)
if (ctrl & APB_DMA_FIN_INT_STS) {
ctrl &= ~APB_DMA_FIN_INT_STS;
if (ch->desc) {
-   spin_lock_irqsave(>vc.lock, flags);
+   spin_lock(>vc.lock);
if (++ch->sgidx < ch->desc->sglen) {
moxart_dma_start_sg(ch, ch->sgidx);
} else {
vchan_cookie_complete(>desc->vd);
moxart_dma_start_desc(>vc.chan);
}
-   spin_unlock_irqrestore(>vc.lock, flags);
+   spin_unlock(>vc.lock);
}
}
 
-- 
2.17.1

[PATCH] drivers:s390:char:move spin_lock_bh to spin_lock in tasklet

[jun qian]

As you are already in a tasklet, it is unnecessary to call spin_lock_bh.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/s390/char/tty3270.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/char/tty3270.c b/drivers/s390/char/tty3270.c
index 5b8af2782282..fb1bc0db7e68 100644
--- a/drivers/s390/char/tty3270.c
+++ b/drivers/s390/char/tty3270.c
@@ -563,7 +563,7 @@ tty3270_read_tasklet(struct raw3270_request *rrq)
char *input;
int len;
 
-   spin_lock_bh(>view.lock);
+   spin_lock(>view.lock);
/*
 * Two AID keys are special: For 0x7d (enter) the input line
 * has to be emitted to the tty and for 0x6d the screen
@@ -590,7 +590,7 @@ tty3270_read_tasklet(struct raw3270_request *rrq)
tp->update_flags = TTY_UPDATE_ALL;
tty3270_set_timer(tp, 1);
}
-   spin_unlock_bh(>view.lock);
+   spin_unlock(>view.lock);
 
/* Start keyboard reset command. */
raw3270_request_reset(tp->kreset);
-- 
2.17.1

[PATCH] drivers:s390:char:move spin_lock_bh to spin_lock in tasklet

[jun qian]

As you are already in a tasklet, it is unnecessary to call spin_lock_bh.

Signed-off-by: jun qian 
Cc: Barry song <21cn...@gmail.com>
---
 drivers/s390/char/tty3270.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/char/tty3270.c b/drivers/s390/char/tty3270.c
index 5b8af2782282..fb1bc0db7e68 100644
--- a/drivers/s390/char/tty3270.c
+++ b/drivers/s390/char/tty3270.c
@@ -563,7 +563,7 @@ tty3270_read_tasklet(struct raw3270_request *rrq)
char *input;
int len;
 
-   spin_lock_bh(>view.lock);
+   spin_lock(>view.lock);
/*
 * Two AID keys are special: For 0x7d (enter) the input line
 * has to be emitted to the tty and for 0x6d the screen
@@ -590,7 +590,7 @@ tty3270_read_tasklet(struct raw3270_request *rrq)
tp->update_flags = TTY_UPDATE_ALL;
tty3270_set_timer(tp, 1);
}
-   spin_unlock_bh(>view.lock);
+   spin_unlock(>view.lock);
 
/* Start keyboard reset command. */
raw3270_request_reset(tp->kreset);
-- 
2.17.1

[PATCH] tty:serial:imx: use spin_lock instead of spin_lock_irqsave in isr

[jun qian]

Before the program enters the uart ISR, the local interrupt has been
disabled by the system, so it's not appropriate to use spin_lock_irqsave
interface in the ISR.

Signed-off-by: jun qian 
---
 drivers/tty/serial/imx.c | 21 -
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 239c0fa2e981..3069ee93583e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -706,27 +706,25 @@ static irqreturn_t imx_uart_rtsint(int irq, void *dev_id)
 {
struct imx_port *sport = dev_id;
u32 usr1;
-   unsigned long flags;
 
-   spin_lock_irqsave(>port.lock, flags);
+   spin_lock(>port.lock);
 
imx_uart_writel(sport, USR1_RTSD, USR1);
usr1 = imx_uart_readl(sport, USR1) & USR1_RTSS;
uart_handle_cts_change(>port, !!usr1);
wake_up_interruptible(>port.state->port.delta_msr_wait);
 
-   spin_unlock_irqrestore(>port.lock, flags);
+   spin_unlock(>port.lock);
return IRQ_HANDLED;
 }
 
 static irqreturn_t imx_uart_txint(int irq, void *dev_id)
 {
struct imx_port *sport = dev_id;
-   unsigned long flags;
 
-   spin_lock_irqsave(>port.lock, flags);
+   spin_lock(>port.lock);
imx_uart_transmit_buffer(sport);
-   spin_unlock_irqrestore(>port.lock, flags);
+   spin_unlock(>port.lock);
return IRQ_HANDLED;
 }
 
@@ -735,9 +733,8 @@ static irqreturn_t imx_uart_rxint(int irq, void *dev_id)
struct imx_port *sport = dev_id;
unsigned int rx, flg, ignored = 0;
struct tty_port *port = >port.state->port;
-   unsigned long flags;
 
-   spin_lock_irqsave(>port.lock, flags);
+   spin_lock(>port.lock);
 
while (imx_uart_readl(sport, USR2) & USR2_RDR) {
u32 usr2;
@@ -797,7 +794,7 @@ static irqreturn_t imx_uart_rxint(int irq, void *dev_id)
}
 
 out:
-   spin_unlock_irqrestore(>port.lock, flags);
+   spin_unlock(>port.lock);
tty_flip_buffer_push(port);
return IRQ_HANDLED;
 }
@@ -903,13 +900,11 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id)
}
 
if (usr1 & USR1_DTRD) {
-   unsigned long flags;
-
imx_uart_writel(sport, USR1_DTRD, USR1);
 
-   spin_lock_irqsave(>port.lock, flags);
+   spin_lock(>port.lock);
imx_uart_mctrl_check(sport);
-   spin_unlock_irqrestore(>port.lock, flags);
+   spin_unlock(>port.lock);
 
ret = IRQ_HANDLED;
}
-- 
2.17.1

[PATCH] tty:serial:imx: use spin_lock instead of spin_lock_irqsave in isr

[jun qian]

Before the program enters the uart ISR, the local interrupt has been
disabled by the system, so it's not appropriate to use spin_lock_irqsave
interface in the ISR.

Signed-off-by: jun qian 
---
 drivers/tty/serial/imx.c | 21 -
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 239c0fa2e981..3069ee93583e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -706,27 +706,25 @@ static irqreturn_t imx_uart_rtsint(int irq, void *dev_id)
 {
struct imx_port *sport = dev_id;
u32 usr1;
-   unsigned long flags;
 
-   spin_lock_irqsave(>port.lock, flags);
+   spin_lock(>port.lock);
 
imx_uart_writel(sport, USR1_RTSD, USR1);
usr1 = imx_uart_readl(sport, USR1) & USR1_RTSS;
uart_handle_cts_change(>port, !!usr1);
wake_up_interruptible(>port.state->port.delta_msr_wait);
 
-   spin_unlock_irqrestore(>port.lock, flags);
+   spin_unlock(>port.lock);
return IRQ_HANDLED;
 }
 
 static irqreturn_t imx_uart_txint(int irq, void *dev_id)
 {
struct imx_port *sport = dev_id;
-   unsigned long flags;
 
-   spin_lock_irqsave(>port.lock, flags);
+   spin_lock(>port.lock);
imx_uart_transmit_buffer(sport);
-   spin_unlock_irqrestore(>port.lock, flags);
+   spin_unlock(>port.lock);
return IRQ_HANDLED;
 }
 
@@ -735,9 +733,8 @@ static irqreturn_t imx_uart_rxint(int irq, void *dev_id)
struct imx_port *sport = dev_id;
unsigned int rx, flg, ignored = 0;
struct tty_port *port = >port.state->port;
-   unsigned long flags;
 
-   spin_lock_irqsave(>port.lock, flags);
+   spin_lock(>port.lock);
 
while (imx_uart_readl(sport, USR2) & USR2_RDR) {
u32 usr2;
@@ -797,7 +794,7 @@ static irqreturn_t imx_uart_rxint(int irq, void *dev_id)
}
 
 out:
-   spin_unlock_irqrestore(>port.lock, flags);
+   spin_unlock(>port.lock);
tty_flip_buffer_push(port);
return IRQ_HANDLED;
 }
@@ -903,13 +900,11 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id)
}
 
if (usr1 & USR1_DTRD) {
-   unsigned long flags;
-
imx_uart_writel(sport, USR1_DTRD, USR1);
 
-   spin_lock_irqsave(>port.lock, flags);
+   spin_lock(>port.lock);
imx_uart_mctrl_check(sport);
-   spin_unlock_irqrestore(>port.lock, flags);
+   spin_unlock(>port.lock);
 
ret = IRQ_HANDLED;
}
-- 
2.17.1