Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Leonard Crestez <leonard.crestez@nxp.com>
To:     Frederic Weisbecker <frederic@kernel.org>
CC:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        Stephen Rothwell <sfr@canb.auug.org.au>
Subject: Re: [PATCH 3/4] irq_work: Slightly simplify IRQ_WORK_PENDING clearing
Thread-Topic: [PATCH 3/4] irq_work: Slightly simplify IRQ_WORK_PENDING
 clearing
Thread-Index: AQHVlk7q8ewqzqG/7U6Uj+DHGZkRhA==
Date:   Tue, 12 Nov 2019 20:27:05 +0000
Message-ID: <VI1PR04MB70239C2E838B72C171CC62C7EE770@VI1PR04MB7023.eurprd04.prod.outlook.com>
References: <20191108160858.31665-1-frederic@kernel.org>
 <20191108160858.31665-4-frederic@kernel.org>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 81ddda22-03e4-4b85-b07a-08d767aeaf71
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Nov 2019 20:27:05.9846
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: +6L7tT7gwxWFeLmj2M6EJx5/VLYdSs9YnN3hAVOazLW6WM6FaxeDF0VAWUS9C1IVx3Hs5au7x4Dm7N/kd/eDsg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5662
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08.11.2019 18:09, Frederic Weisbecker wrote:=0A=
> Instead of fetching the value of flags and perform an xchg() to clear=0A=
> a bit, just use atomic_fetch_andnot() that is more suitable to do that=0A=
> job in one operation while keeping the full ordering.=0A=
> =0A=
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>=0A=
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>=0A=
> Cc: Thomas Gleixner <tglx@linutronix.de>=0A=
> Cc: Peter Zijlstra <peterz@infradead.org>=0A=
> Cc: Ingo Molnar <mingo@kernel.org>=0A=
> ---=0A=
>   kernel/irq_work.c | 7 +++----=0A=
>   1 file changed, 3 insertions(+), 4 deletions(-)=0A=
> =0A=
> diff --git a/kernel/irq_work.c b/kernel/irq_work.c=0A=
> index 255454a48346..49c53f80a13a 100644=0A=
> --- a/kernel/irq_work.c=0A=
> +++ b/kernel/irq_work.c=0A=
> @@ -34,7 +34,7 @@ static bool irq_work_claim(struct irq_work *work)=0A=
>   	oflags =3D atomic_fetch_or(IRQ_WORK_CLAIMED, &work->flags);=0A=
>   	/*=0A=
>   	 * If the work is already pending, no need to raise the IPI.=0A=
> -	 * The pairing atomic_xchg() in irq_work_run() makes sure=0A=
> +	 * The pairing atomic_fetch_andnot() in irq_work_run() makes sure=0A=
>   	 * everything we did before is visible.=0A=
>   	 */=0A=
>   	if (oflags & IRQ_WORK_PENDING)=0A=
> @@ -135,7 +135,6 @@ static void irq_work_run_list(struct llist_head *list=
)=0A=
>   {=0A=
>   	struct irq_work *work, *tmp;=0A=
>   	struct llist_node *llnode;=0A=
> -	int flags;=0A=
>   =0A=
>   	BUG_ON(!irqs_disabled());=0A=
>   =0A=
> @@ -144,6 +143,7 @@ static void irq_work_run_list(struct llist_head *list=
)=0A=
>   =0A=
>   	llnode =3D llist_del_all(list);=0A=
>   	llist_for_each_entry_safe(work, tmp, llnode, llnode) {=0A=
> +		int flags;=0A=
>   		/*=0A=
>   		 * Clear the PENDING bit, after this point the @work=0A=
>   		 * can be re-used.=0A=
> @@ -151,8 +151,7 @@ static void irq_work_run_list(struct llist_head *list=
)=0A=
>   		 * to claim that work don't rely on us to handle their data=0A=
>   		 * while we are in the middle of the func.=0A=
>   		 */=0A=
> -		flags =3D atomic_read(&work->flags) & ~IRQ_WORK_PENDING;=0A=
> -		atomic_xchg(&work->flags, flags);=0A=
> +		flags =3D atomic_fetch_andnot(IRQ_WORK_PENDING, &work->flags);=0A=
>   =0A=
>   		work->func(work);=0A=
>   		/*=0A=
=0A=
This breaks switching between cpufreq governors in linux-next on arm64 =0A=
and various other stuff. The fix in this email doesn't compile:=0A=
=0A=
     https://lkml.org/lkml/2019/11/12/622=0A=
=0A=
I assume you meant "&=3D ~" instead of "~=3D", this seems to work:=0A=
=0A=
diff --git a/kernel/irq_work.c b/kernel/irq_work.c=0A=
index 49c53f80a13a..828cc30774bc 100644=0A=
--- a/kernel/irq_work.c=0A=
+++ b/kernel/irq_work.c=0A=
@@ -156,10 +156,11 @@ static void irq_work_run_list(struct llist_head *list=
)=0A=
                 work->func(work);=0A=
                 /*=0A=
                  * Clear the BUSY bit and return to the free state if=0A=
                  * no-one else claimed it meanwhile.=0A=
                  */=0A=
+               flags &=3D ~IRQ_WORK_PENDING;=0A=
                 (void)atomic_cmpxchg(&work->flags, flags, flags & =0A=
~IRQ_WORK_BUSY);=0A=
         }=0A=
  }=0A=