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

[Jonathan Cameron]

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.
> 
> :-(
> 
> 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