Re: [PATCH v8 12/14] powerpc/vas: Return credits after handling fault

[Haren Myneni <haren@linux.ibm.com>]

On Mon, 2020-03-23 at 12:44 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:19 pm:
> > 
> > NX expects OS to return credit for send window after processing each
> > fault. Also credit has to be returned even for fault window.
> 
> And this should be merged in the fault handler function.

credits are assigned and used per VAS window - default value is 1024 for
user space windows, and fault_fifo_size/CRb_SIZE for fault window.
 
When user space submits request, credit is taken on specific window (by
VAS). After successful processing of this request, NX return credit. In
case if NX sees fault, expects OS return credit for the corresponding
user space window after handling fault CRB. 

Similarly NX takes credit on fault window after pasting fault CRB and
expects return credit after handling fault CRB. NX workbook has on
credits usage and How this credit system works.

Thought vas_return_credit() is unique function and added as separate
patch so that easy to review.

> 
> > 
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> >  arch/powerpc/platforms/powernv/vas-fault.c  |  9 +++++++++
> >  arch/powerpc/platforms/powernv/vas-window.c | 17 +++++++++++++++++
> >  arch/powerpc/platforms/powernv/vas.h        |  1 +
> >  3 files changed, 27 insertions(+)
> > 
> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> > index 40e1de4..292f7ba 100644
> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> > @@ -238,6 +238,10 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
> >  		memcpy(crb, fifo, CRB_SIZE);
> >  		entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
> >  		entry->ccw |= cpu_to_be32(CCW0_INVALID);
> > +		/*
> > +		 * Return credit for the fault window.
> > +		 */
> 
> None of the comments in this patch are useful.
> 
> > +		vas_return_credit(vinst->fault_win, 0);
> 
> Can you use true/false for bools?
> 
> >  		mutex_unlock(&vinst->mutex);
> >  
> >  		pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
> > @@ -267,6 +271,11 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
> >  		}
> >  
> >  		update_csb(window, crb);
> > +		/*
> > +		 * Return credit for send window after processing
> > +		 * fault CRB.
> > +		 */
> 
> Any chance of a little bit of explanation how the credit system works?
> Or is it in the code somewhere already?
Sure will add few comments on credit usage.
> 
> I don't suppose there is a chance to batch credit updates with multiple
> faults? (maybe the MMIO is insignificant)

Yes, we return credit after processing each CRB. In the case of fault
window, NX can continue pasting fault CRB whenever the credit is
available. 

Thanks
Haren

> 
> > +		vas_return_credit(window, 1);
> >  	} while (true);
> >  }
> >  
> 
> Thanks,
> Nick