Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 20 Nov 2022 05:57:22 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Hillf Danton <hdanton@sina.com>
CC:     Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Li Ming <ming4.li@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Lukas Wunner <lukas@wunner.de>,
        Alison Schofield <alison.schofield@intel.com>,
        <linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>
Subject: Re: [PATCH] PCI/DOE: Remove asynchronous task support
Message-ID: <Y3oyQhHUbYZGA80M@iweiny-mobl>
References: <20221119222527.1799836-1-ira.weiny@intel.com>
 <20221120022735.4671-1-hdanton@sina.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221120022735.4671-1-hdanton@sina.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?y0z90xXHq8iOLu9C2FRhJtIbDD3bnklOMdqEOVgZIngocx7B666m+OXbMD3N?=
 =?us-ascii?Q?TDth6RogWTiPr5xtOc2ePFyGUcT1LcUJbcJEeMgE6EulA4r4++K+XDsDKj4S?=
 =?us-ascii?Q?UMuYVWpmugwL3huOOFh8/2ci+sZeb5xsWDNZDi7Pci41uW3rFxLwIfya6DkN?=
 =?us-ascii?Q?Melx/JSDjXoTFqYj4nko7SLvoJ7BykOgOod+n1rw8n4fk/Jkw0XXUs02UdNh?=
 =?us-ascii?Q?QKe2goxGRi4tHXQRRg3PGjV63peBSMsltYvOy3Pk3ImGWUrl5MOfWeV2RejD?=
 =?us-ascii?Q?T5yGVLGIK6vLmQbOqIhcKqEGqM0Co1faXS6rquHTDl8YbSmse4LPb51AdMDP?=
 =?us-ascii?Q?m8YvPhVMI8MErxSoRGchFY7WdtqmHDOInQDvrSFo2nOGyubRNPZaRw0MDlBl?=
 =?us-ascii?Q?aG1ob6V1/+EAKzQshfqCMxoKuvIfkhS5vmu05puzGQRwk4b9lmWAWq97qBwa?=
 =?us-ascii?Q?ciUIWqqLIJhAOKzgQNjomc6NH3vBozvRmcZOqK2Gsa86CAxIUoAR/LnIo3EL?=
 =?us-ascii?Q?YTNwiGojFx6O61ySYAYe63eqaTRotJrTQhdlmSdC6/Qp6cvmcSHpXrwdfX7j?=
 =?us-ascii?Q?JTroAA+G6ea4txVyiKcnUBVBzOPyBxRTYvBcFBP9akj5rJe+oiB8SNZVxDrv?=
 =?us-ascii?Q?4CaWc6gGVF4qli4O9bMtkn+Xv5Hwogd0ty6yqEMKwat4fhuYNZx8KBOK1Y1F?=
 =?us-ascii?Q?cJaxMcwDH7fohT3gkTl6uxTtg6gi5b8L0Z65UO4bwWLMYbWh0FyDLtaqL0GV?=
 =?us-ascii?Q?jiE7c39TTbdKIZshgAUMZ7z3kqqYfXt4bP/3WvQM+qJnxt9rwNwgbOAvgVXe?=
 =?us-ascii?Q?VxYwJwIpoZGqoev6QwniYSUAhfyaoF8aeyksZEMuMo6WERmOibE5y9WDRx2z?=
 =?us-ascii?Q?4va7G5n15CikQCYUi8s9dXqJJWFjHV5cXnQrUMQe/w57F3NdaB7RdwnRlOWv?=
 =?us-ascii?Q?bdzCepdJ7h0PpEAl+MINXaBbJCPf+m+jTEbt1/Q1PJOpoMV7Nh/j+yvXgAMx?=
 =?us-ascii?Q?hixsKNiS0/HsOyyvkWSYygNP5CPubZbxbKNf7i5V8kfbgwiFwRmfJqCSnor0?=
 =?us-ascii?Q?Lp1wurd5RAeSvuGR9avvG9ZsSKwSUYKr6Ty+rg/pBND6cZ8j8fzeZ/pUD37d?=
 =?us-ascii?Q?dptHX9ZVdrucW+gqdgJ/h3YB6fgmUK/RGt6DxWLCUAa9Jv4ut/hTBxOdTFAS?=
 =?us-ascii?Q?9SYspbq+P/rfKcpFpGHjT6/UCPaIFqRYTGTJqcRgH45dUgj32sjEgwyjpEpD?=
 =?us-ascii?Q?cuO3zXz1p+Ksda7zRIoAZCTOV8j8L81jP9vXx9FjL4bFONjaeF/OQebaeKlz?=
 =?us-ascii?Q?S259+aBFYKxXcX7e/aA6JUqeDlVa8kKefdbESfab1XvidjbjBMXec9L3gvbL?=
 =?us-ascii?Q?k7gzEPM8a2gx9Zcp/aZDIClzOpTU7vfe3Y5bDg5wnOXY6dcVLqDgva0AJG92?=
 =?us-ascii?Q?rvtC2QFbbqogjhwSbh9qJaP70Sbt7EpHBBksNq+6jzxXHUuwNAU50OYeKW5T?=
 =?us-ascii?Q?/9mIGbcllOI1y0rN6FCK09pJWyYADO22OC0ORvIVu+kj55D8O4l4Ms4wNEC/?=
 =?us-ascii?Q?UN5GSKWMQX8CccE3glTLd84MRgmv96Wp/GN1V68p?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bb235bc5-a964-4246-bda5-08dacaff2852
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Nov 2022 13:57:28.1462
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kd05PrsYHhOczadlPoTgZVRDHnUw/gsXGYkQ3nmrK9HKzpXrsxUFO/sMDl7SZ9ALXrrF/LpCn0/2HSC9z1Q5GQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5971
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Nov 20, 2022 at 10:27:35AM +0800, Hillf Danton wrote:
> On Sat, 19 Nov 2022 14:25:27 -0800 Ira Weiny <ira.weiny@intel.com>
> > @@ -529,8 +492,18 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
> >  		return -EIO;
> >  
> >  	task->doe_mb = doe_mb;
> > -	INIT_WORK(&task->work, doe_statemachine_work);
> > -	queue_work(doe_mb->work_queue, &task->work);
> > +
> > +again:
> > +	if (!mutex_trylock(&doe_mb->exec_lock)) {
> > +		if (wait_event_timeout(task->doe_mb->wq,
> > +				test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
> > +				PCI_DOE_POLL_INTERVAL))
> > +			return -EIO;
> 
> Is EIO worth a line of pr_warn()?

Maybe but I'm not sure it is worth it.  This was paralleling the original code
which called pci_doe_flush_mb() to shut down the mailbox.  So this is likely to
never happen.  The callers could print something if needed.

> 
> > +		goto again;
> > +	}
> > +	exec_task(task);
> > +	mutex_unlock(&doe_mb->exec_lock);
> > +
> 
> If it is likely to take two minutes to acquire the exec_lock after
> rounds of trying again, trylock + wait timeout barely make sense given EIO.

I'm not sure where 2 minutes come from?

#define PCI_DOE_TIMEOUT HZ
#define PCI_DOE_POLL_INTERVAL   (PCI_DOE_TIMEOUT / 128)

It is also not anticipated that more than 1 task is being given to the mailbox
but the protection needs to be there because exec_task() will get confused if
more than 1 thread submits at the same time.

All this said I've now convinced myself that there is a race in the use of
PCI_DOE_FLAG_CANCEL even with the existing code.

I believe that if the pci device goes away the doe_mb structure may get free'ed
prior to other threads having a chance to check doe_mb->flags.  Worse yet the
work queue itself (doe_mb->wq) may become invalid...

I don't believe this can currently happen because anyone using the doe_mb
structure has a reference to the pci device.

With this patch I think all the doe_mb->flags and the wait queue can go away.
pci_doe_wait() can be replaced with a simple msleep_interruptible().

Let me work through that a bit.

Ira

> 
> Hillf
> 
> /**
>  * wait_event_timeout - sleep until a condition gets true or a timeout elapses
>  * @wq_head: the waitqueue to wait on
>  * @condition: a C expression for the event to wait for
>  * @timeout: timeout, in jiffies
>  *
>  * The process is put to sleep (TASK_UNINTERRUPTIBLE) until the
>  * @condition evaluates to true. The @condition is checked each time
>  * the waitqueue @wq_head is woken up.
>  *
>  * wake_up() has to be called after changing any variable that could
>  * change the result of the wait condition.
>  *
>  * Returns:
>  * 0 if the @condition evaluated to %false after the @timeout elapsed,
>  * 1 if the @condition evaluated to %true after the @timeout elapsed,
>  * or the remaining jiffies (at least 1) if the @condition evaluated
>  * to %true before the @timeout elapsed.
>  */