Re: [V3 PATCH 03/16] powerpc/vas: Create take/drop task reference functions

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

On Mon, 2021-05-10 at 15:28 +1000, Nicholas Piggin wrote:
> Excerpts from Haren Myneni's message of April 18, 2021 7:03 am:
> > Take task reference when each window opens and drops during close.
> > This functionality is needed for powerNV and pseries. So this patch
> > defines the existing code as functions in common book3s platform
> > vas-api.c
> > 
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> >  arch/powerpc/include/asm/vas.h              | 20 ++++++++
> >  arch/powerpc/platforms/book3s/vas-api.c     | 51
> > ++++++++++++++++++
> >  arch/powerpc/platforms/powernv/vas-fault.c  | 10 ++--
> >  arch/powerpc/platforms/powernv/vas-window.c | 57 ++---------------
> > ----
> >  arch/powerpc/platforms/powernv/vas.h        |  6 +--
> >  5 files changed, 83 insertions(+), 61 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/vas.h
> > b/arch/powerpc/include/asm/vas.h
> > index 6bbade60d8f4..2daaa1a2a9a9 100644
> > --- a/arch/powerpc/include/asm/vas.h
> > +++ b/arch/powerpc/include/asm/vas.h
> > @@ -5,6 +5,9 @@
> >  
> >  #ifndef _ASM_POWERPC_VAS_H
> >  #define _ASM_POWERPC_VAS_H
> > +#include <linux/sched/mm.h>
> > +#include <linux/mmu_context.h>
> > +#include <asm/icswx.h>
> >  #include <uapi/asm/vas-api.h>
> >  
> >  
> > @@ -60,6 +63,22 @@ struct vas_user_win_ops {
> >  	int (*close_win)(void *);
> >  };
> >  
> > +struct vas_win_task {
> > +	struct pid *pid;	/* Thread group ID of owner */
> > +	struct pid *tgid;	/* Linux process mm_struct */
> > +	struct mm_struct *mm;	/* Linux process mm_struct */
> > +};
> 
> Looks okay, happy with struct vas_win_task? (and vas_user_win_ops)?
> 
> I'd be happier to have everything related to vas windows prefixed
> with 
> vas_window_ consistently, and have _user be present always for
> userspace
> windows, but you have to read and type it.

I will change to vas_window_task and vas_user_window_ops. struct
vas_window is common for both kernel and user space window, but struct
vas_window_task is needed only for user space.

> 
> > +
> > +static inline void vas_drop_reference_task(struct vas_win_task
> > *task)
> 
> This is not dropping a reference task, but a task reference. And
> it's 
> not really a task reference as far as Linux understands, but a
> reference on pid (not task) and mm related to an open vas window. And
> specifically a user window (with corresponding vas_user_win_ops).

Yes get and put references to pid and mm. Hence mentioned
reference_task to reflect for both pid and mm. Would you prefer
vas_put_reference_pid_mm()?

> 
> Could it be called a 'struct vas_window_user_ref' instead?

To support LPM and DLPAR, plannig to add VMA (mapping address) in
struct vas_window_task. But I can change it to 'struct
vas_window_user_ref' instread of vas_window_task.

Thanks
Haren 

> 
> Thanks,
> Nick
> 
> > +{
> > +	/* Drop references to pid and mm */
> > +	put_pid(task->pid);
> > +	if (task->mm) {
> > +		mm_context_remove_vas_window(task->mm);
> > +		mmdrop(task->mm);
> > +	}