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:   Tue, 22 Nov 2022 07:55:14 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     "Li, Ming" <ming4.li@intel.com>,
        Gregory Price <gregory.price@memverge.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Lukas Wunner" <lukas@wunner.de>,
        Alison Schofield <alison.schofield@intel.com>,
        <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-pci@vger.kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        "Bjorn Helgaas" <bhelgaas@google.com>
Subject: Re: [PATCH] PCI/DOE: Remove asynchronous task support
Message-ID: <Y3zw4vdR7pYPaMen@iweiny-mobl>
References: <20221119222527.1799836-1-ira.weiny@intel.com>
 <750c2096-fe2f-0597-a635-78cbe6b0ee0d@intel.com>
 <e7db6aea-4146-33f2-9490-9b5b902e0ec1@intel.com>
 <Y3wC4kX6SCr90FGY@iweiny-desk3>
 <20221122094627.00003f2c@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221122094627.00003f2c@Huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?46vTowarSAogap3Efmyp0Gc5VeVUPNunGecoet/97it+xfnIhqFNdnRmGwNN?=
 =?us-ascii?Q?ecH6bxhMITyreQ8yMS30nP+LHy9qLdhoPtTSoXJrW0zHkRZw/2NB4oJpDE9A?=
 =?us-ascii?Q?hks3al0V3Q53m3ye/wyAICQdzsADGP8XjxvkoSKGsmbSgm94eOWGhhKv83yc?=
 =?us-ascii?Q?gnhkxRPqL7UCv3GZx5odwlsrXq/dwaRNsuXOtEGFg3E60e1s2oiOUfNkZ2QU?=
 =?us-ascii?Q?BjbCPjIO0Pqq4Vy5RLDDiZNFI5S7NzQ3hH4DQLC/3Ivce0dy2u2jTQ0f8apH?=
 =?us-ascii?Q?+BU8iooTz9VgQAk9XghgXsLaDCzhpSGZZQwzhjrL+bkOfZuYFz6IKm8CTAMR?=
 =?us-ascii?Q?jgLF4PImcd9mygKyQC+KSvJVHGDdPnMhAFAdoLb1282tRe4LTyC+m1BXdv8F?=
 =?us-ascii?Q?xWP7AxgPE40C8aJJ3z8kNeOunFlunRW/onpoQBw7TQ0MDuE9bPjNx5VyOJsm?=
 =?us-ascii?Q?zcxPNo98CYIqhfzEgATbL0f2LuUTb6SHkhcivtDKr4pRc5IF6ks+X1q76rHv?=
 =?us-ascii?Q?V5FX9GKBn8Ayeh49G5tOZTjaD/hXr8Um5H7pvV6vuk7kQMfuF2aSVRyl7JIA?=
 =?us-ascii?Q?eroGi4wwdJvbNlJTbeJnhumTbE0l1nXSrfa4BtoxWgLwf1jPKEWBP249urFs?=
 =?us-ascii?Q?znKXSBrfzmMwF2DxpPNmli2CKewg8QZPKNhbYc4SABDhUSPZoqdwwj2ntHHC?=
 =?us-ascii?Q?r0ADRyWNwyZYNk1P6anMDOH89brjuJ6tSKat6vr6C9pq3hr22t0byJO2ISPB?=
 =?us-ascii?Q?sPdTjnTVQ9rQzhv7rkGpr+P02WMMVw1FS5oMUtj4lJvfNDmpfjjaRbyiwasM?=
 =?us-ascii?Q?vFs036jSlH3v1u9jAosT2XbrAJdJDtYjnyugNtuk1VSY+JAD9U5dx0qmiVp7?=
 =?us-ascii?Q?RSN8ZSBtmSVH7P7BH1AnAxh+meqRIBPD8RwnPD9b4HbvUTtdAKjuNkXoaCq2?=
 =?us-ascii?Q?8hLAep9/t1Zx3rZAxqlFFL5+dmjnb05q0hvQAWsNCJBWA62nUCaf/6amEp0A?=
 =?us-ascii?Q?uMQkW/cHD/gUUBatPD1Rv+XW/2ZRjWhL2iP56giTtEu9TR1p9+wrSZvgJlQn?=
 =?us-ascii?Q?hsFdEUgpTsOKzqrXa+xj4uETQZpN1D/MNB1V/rn0LnJtU6wOYOP9qcPjI3bX?=
 =?us-ascii?Q?OiNOcZONrQEYaU1J75d3gaUVHDfD1oaRS2to/csHuVxiRl2w5vXf+03Lmn1i?=
 =?us-ascii?Q?w/GqOr/fCFhdHUFD45gfofENPEx7ZKmHdYkJ5bCeI9b7Us6SI6bMi4kyxEdK?=
 =?us-ascii?Q?ozlRV+Qo/SwCZnHZrzaGLI/EBzSW6q5a8l9rl3v9quMsLiAvow3eq7jS0tYA?=
 =?us-ascii?Q?q2vkNJhTcF7HGV8oAmsKjNmE5ZN00NWo7eVF7Mpl0e0Xf7p6fE9M0Q3cyA7B?=
 =?us-ascii?Q?J4tfXB3W8RLHD/MlM0d4D198h3JqFpGtilsLJSYN5NyR/h6euFsUfCBrvuXB?=
 =?us-ascii?Q?vXRLBB/EcbBGTFVNfCmq2Qd+IW/H83feR3mNy23zNK+jWEINtgteehSWgbgR?=
 =?us-ascii?Q?AdENMGCgCGTdoBgWPsWx5a6xOHl9z+AbYYnik5q0dBwJcnaXhVCvnNItChP+?=
 =?us-ascii?Q?9oXnMis+EuVIxw2EVU7VsK3nruXQn0VROzYDoEjd?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e62407eb-8ed8-44a6-d36d-08dacca1f40f
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Nov 2022 15:55:18.4387
 (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: 1BzQthY2ZeYEmN4PfDTD4FmU7RRHBhMfcZpcZkGQCOwxEcZaebFMwGqY2qBFpA8jta8BAIBl0YK4+dRJUEZclg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4542
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,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,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 Tue, Nov 22, 2022 at 09:46:27AM +0000, Jonathan Cameron wrote:
> On Mon, 21 Nov 2022 14:59:46 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > On Mon, Nov 21, 2022 at 10:07:56AM +0800, Li, Ming wrote:
> > > On 11/21/2022 9:39 AM, Li, Ming wrote:  
> > 
> > [snip]
> > 
> > > >> @@ -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;  
> > > > 
> > > > We already implemented a pci_doe_wait(), I think we can use it to instead of this wait_event_timeout.
> > > > 
> > > > Thanks
> > > > Ming
> > > >   
> > > 
> > > This wait_event_timeout() only check PCI_DOE_FLAG_CANCEL, that means it only detects the signal which the doe_mb has being destroyed.
> > > If current doe task is done correctly, I think we should wake up next task. Current implementation just waits utill timeout happens and try it again.
> > > Besides, If two threads are waiting a same doe_mb, thread #1 waited firstly, thread #2 waited secondly, there is a chance that thread #2 is processed before thread #1.
> > >   
> > 
> > Agreed.
> > 
> > However, the real problem is that the doe_mb is probably free'ed at this point
> > and all this is going to crash and burn anyway.  The implementation of
> > PCI_DOE_FLAG_CANCEL was fundamentally flawed even for the current work queue
> > implementation.
> > 
> > This patch incorrectly tried to use that mechanism but upon looking closer I
> > see it does not work.
> > 
> > I saw in another thread Jonathan discussing some sort of get/put on the doe_mb.
> > That is not currently necessary as the creators of doe_mb objects currently
> > hold references to the PCI device any time they call submit.
> 
> The get / put would only matter if we wanted to manage the DOE resources separately
> from those of the PCI device.  It may well never make sense to do so as they
> aren't substantial anyway.

Agreed.  See the new series:

https://lore.kernel.org/all/20221122155324.1878416-1-ira.weiny@intel.com/

Thanks,
Ira

> > 
> > :-(
> > 
> > For now all PCI_DOE_FLAG_CANCEL stuff needs to go away,
> > Ira
> > 
> > > Thanks
> > > Ming
> > >   
> > > >> +		goto again;
> > > >> +	}
> > > >> +	exec_task(task);
> > > >> +	mutex_unlock(&doe_mb->exec_lock);
> > > >> +
> > > >>  	return 0;
> > > >>  }
> > > >> -EXPORT_SYMBOL_GPL(pci_doe_submit_task);
> > > >> +EXPORT_SYMBOL_GPL(pci_doe_submit_task_wait);
> > > >> diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
> > > >> index ed9b4df792b8..c94122a66221 100644
> > > >> --- a/include/linux/pci-doe.h
> > > >> +++ b/include/linux/pci-doe.h
> > > >> @@ -30,8 +30,6 @@ struct pci_doe_mb;
> > > >>   * @response_pl_sz: Size of the response payload (bytes)
> > > >>   * @rv: Return value.  Length of received response or error (bytes)
> > > >>   * @complete: Called when task is complete
> > > >> - * @private: Private data for the consumer
> > > >> - * @work: Used internally by the mailbox
> > > >>   * @doe_mb: Used internally by the mailbox
> > > >>   *
> > > >>   * The payload sizes and rv are specified in bytes with the following
> > > >> @@ -50,11 +48,6 @@ struct pci_doe_task {
> > > >>  	u32 *response_pl;
> > > >>  	size_t response_pl_sz;
> > > >>  	int rv;
> > > >> -	void (*complete)(struct pci_doe_task *task);
> > > >> -	void *private;
> > > >> -
> > > >> -	/* No need for the user to initialize these fields */
> > > >> -	struct work_struct work;
> > > >>  	struct pci_doe_mb *doe_mb;
> > > >>  };
> > > >>  
> > > >> @@ -72,6 +65,5 @@ struct pci_doe_task {
> > > >>  
> > > >>  struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset);
> > > >>  bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type);
> > > >> -int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
> > > >> -
> > > >> +int pci_doe_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
> > > >>  #endif
> > > >>
> > > >> base-commit: b6e7fdfd6f6a8bf88fcdb4a45da52c42ba238c25  
>