Re: [PATCH] PCI/DOE: Remove asynchronous task support

[Ira Weiny]

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@xxxxxxxxx> 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@xxxxxxxxx/

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  
>