[v2,8/9] drm/fdinfo: Add comm/cmdline override fields

Commit Message

Rob Clark April 27, 2023, 5:53 p.m. UTC

From: Rob Clark <[e-mail]>

These are useful in particular for VM scenarios where the process which
has opened to drm device file is just a proxy for the real user in a VM
guest.

Signed-off-by: Rob Clark <[e-mail]>
---
 Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++
 drivers/gpu/drm/drm_file.c            | 15 +++++++++++++++
 include/drm/drm_file.h                | 19 +++++++++++++++++++
 3 files changed, 52 insertions(+)

Comments

Tvrtko Ursulin April 28, 2023, 11:05 a.m. UTC | #1

On 27/04/2023 18:53, Rob Clark wrote:
> From: Rob Clark <[e-mail]>
> 
> These are useful in particular for VM scenarios where the process which
> has opened to drm device file is just a proxy for the real user in a VM
> guest.
> 
> Signed-off-by: Rob Clark <[e-mail]>
> ---
>   Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++
>   drivers/gpu/drm/drm_file.c            | 15 +++++++++++++++
>   include/drm/drm_file.h                | 19 +++++++++++++++++++
>   3 files changed, 52 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> index 58dc0d3f8c58..e4877cf8089c 100644
> --- a/Documentation/gpu/drm-usage-stats.rst
> +++ b/Documentation/gpu/drm-usage-stats.rst
> @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` shall be present as well.
>   Userspace should make sure to not double account any usage statistics by using
>   the above described criteria in order to associate data to individual clients.
>   
> +- drm-comm-override: <valstr>
> +
> +Returns the client executable override string.  Some drivers support letting
> +userspace override this in cases where the userspace is simply a "proxy".
> +Such as is the case with virglrenderer drm native context, where the host
> +process is just forwarding command submission, etc, from guest userspace.
> +This allows the proxy to make visible the executable name of the actual
> +app in the VM guest.
> +
> +- drm-cmdline-override: <valstr>
> +
> +Returns the client cmdline override string.  Some drivers support letting
> +userspace override this in cases where the userspace is simply a "proxy".
> +Such as is the case with virglrenderer drm native context, where the host
> +process is just forwarding command submission, etc, from guest userspace.
> +This allows the proxy to make visible the cmdline of the actual app in the
> +VM guest.

Perhaps it would be okay to save space here by not repeating the 
description, like:

drm-comm-override: <valstr>
drm-cmdline-override: <valstr>

Long description blah blah...
This allows the proxy to make visible the _executable name *and* command 
line_ blah blah..

> +
>   Utilization
>   ^^^^^^^^^^^
>   
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 9321eb0bf020..d7514c313af1 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>   	spin_lock_init(&file->master_lookup_lock);
>   	mutex_init(&file->event_read_lock);
>   
> +	mutex_init(&file->override_lock);
> +
>   	if (drm_core_check_feature(dev, DRIVER_GEM))
>   		drm_gem_open(dev, file);
>   
> @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file)
>   	WARN_ON(!list_empty(&file->event_list));
>   
>   	put_pid(file->pid);
> +	kfree(file->override_comm);
> +	kfree(file->override_cmdline);
>   	kfree(file);
>   }
>   
> @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f)
>   			   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>   	}
>   
> +	mutex_lock(&file->override_lock);

You could add a fast unlocked check before taking the mutex for no risk 
apart a transient false negative. For 99.9999% of userspace it would 
mean no pointless lock/unlock cycle.

> +	if (file->override_comm) {
> +		drm_printf(&p, "drm-comm-override:\t%s\n",
> +			   file->override_comm);
> +	}
> +	if (file->override_cmdline) {
> +		drm_printf(&p, "drm-cmdline-override:\t%s\n",
> +			   file->override_cmdline);
> +	}
> +	mutex_unlock(&file->override_lock);
> +
>   	if (dev->driver->show_fdinfo)
>   		dev->driver->show_fdinfo(&p, file);
>   }
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 1339e925af52..604d05fa6f0c 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -370,6 +370,25 @@ struct drm_file {
>   	 */
>   	struct drm_prime_file_private prime;
>   
> +	/**
> +	 * @comm: Overridden task comm
> +	 *
> +	 * Accessed under override_lock
> +	 */
> +	char *override_comm;
> +
> +	/**
> +	 * @cmdline: Overridden task cmdline
> +	 *
> +	 * Accessed under override_lock
> +	 */
> +	char *override_cmdline;
> +
> +	/**
> +	 * @override_lock: Serialize access to override_comm and override_cmdline
> +	 */
> +	struct mutex override_lock;
> +

I don't think this should go to drm just yet though. Only one driver can 
make use of it so I'd leave it for later and print from msm_show_fdinfo 
for now.

Regards,

Tvrtko

>   	/* private: */
>   #if IS_ENABLED(CONFIG_DRM_LEGACY)
>   	unsigned long lock_count; /* DRI1 legacy lock count */

Rob Clark May 1, 2023, 4:58 p.m. UTC | #2

On Fri, Apr 28, 2023 at 4:05 AM Tvrtko Ursulin
<[e-mail]> wrote:
>
>
> On 27/04/2023 18:53, Rob Clark wrote:
> > From: Rob Clark <[e-mail]>
> >
> > These are useful in particular for VM scenarios where the process which
> > has opened to drm device file is just a proxy for the real user in a VM
> > guest.
> >
> > Signed-off-by: Rob Clark <[e-mail]>
> > ---
> >   Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++
> >   drivers/gpu/drm/drm_file.c            | 15 +++++++++++++++
> >   include/drm/drm_file.h                | 19 +++++++++++++++++++
> >   3 files changed, 52 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> > index 58dc0d3f8c58..e4877cf8089c 100644
> > --- a/Documentation/gpu/drm-usage-stats.rst
> > +++ b/Documentation/gpu/drm-usage-stats.rst
> > @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` shall be present as well.
> >   Userspace should make sure to not double account any usage statistics by using
> >   the above described criteria in order to associate data to individual clients.
> >
> > +- drm-comm-override: <valstr>
> > +
> > +Returns the client executable override string.  Some drivers support letting
> > +userspace override this in cases where the userspace is simply a "proxy".
> > +Such as is the case with virglrenderer drm native context, where the host
> > +process is just forwarding command submission, etc, from guest userspace.
> > +This allows the proxy to make visible the executable name of the actual
> > +app in the VM guest.
> > +
> > +- drm-cmdline-override: <valstr>
> > +
> > +Returns the client cmdline override string.  Some drivers support letting
> > +userspace override this in cases where the userspace is simply a "proxy".
> > +Such as is the case with virglrenderer drm native context, where the host
> > +process is just forwarding command submission, etc, from guest userspace.
> > +This allows the proxy to make visible the cmdline of the actual app in the
> > +VM guest.
>
> Perhaps it would be okay to save space here by not repeating the
> description, like:
>
> drm-comm-override: <valstr>
> drm-cmdline-override: <valstr>
>
> Long description blah blah...
> This allows the proxy to make visible the _executable name *and* command
> line_ blah blah..
>
> > +
> >   Utilization
> >   ^^^^^^^^^^^
> >
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 9321eb0bf020..d7514c313af1 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
> >       spin_lock_init(&file->master_lookup_lock);
> >       mutex_init(&file->event_read_lock);
> >
> > +     mutex_init(&file->override_lock);
> > +
> >       if (drm_core_check_feature(dev, DRIVER_GEM))
> >               drm_gem_open(dev, file);
> >
> > @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file)
> >       WARN_ON(!list_empty(&file->event_list));
> >
> >       put_pid(file->pid);
> > +     kfree(file->override_comm);
> > +     kfree(file->override_cmdline);
> >       kfree(file);
> >   }
> >
> > @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f)
> >                          PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> >       }
> >
> > +     mutex_lock(&file->override_lock);
>
> You could add a fast unlocked check before taking the mutex for no risk
> apart a transient false negative. For 99.9999% of userspace it would
> mean no pointless lock/unlock cycle.

I'm not sure I get your point?  This needs to be serialized against
userspace setting the override values

>
> > +     if (file->override_comm) {
> > +             drm_printf(&p, "drm-comm-override:\t%s\n",
> > +                        file->override_comm);
> > +     }
> > +     if (file->override_cmdline) {
> > +             drm_printf(&p, "drm-cmdline-override:\t%s\n",
> > +                        file->override_cmdline);
> > +     }
> > +     mutex_unlock(&file->override_lock);
> > +
> >       if (dev->driver->show_fdinfo)
> >               dev->driver->show_fdinfo(&p, file);
> >   }
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index 1339e925af52..604d05fa6f0c 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -370,6 +370,25 @@ struct drm_file {
> >        */
> >       struct drm_prime_file_private prime;
> >
> > +     /**
> > +      * @comm: Overridden task comm
> > +      *
> > +      * Accessed under override_lock
> > +      */
> > +     char *override_comm;
> > +
> > +     /**
> > +      * @cmdline: Overridden task cmdline
> > +      *
> > +      * Accessed under override_lock
> > +      */
> > +     char *override_cmdline;
> > +
> > +     /**
> > +      * @override_lock: Serialize access to override_comm and override_cmdline
> > +      */
> > +     struct mutex override_lock;
> > +
>
> I don't think this should go to drm just yet though. Only one driver can
> make use of it so I'd leave it for later and print from msm_show_fdinfo
> for now.

This was my original approach but danvet asked that it be moved into
drm for consistency across drivers.  (And really, I want the in-flight
amd and intel native-context stuff to motivate adding similar features
to amdgpu/i915/xe.)

BR,
-R

> Regards,
>
> Tvrtko
>
> >       /* private: */
> >   #if IS_ENABLED(CONFIG_DRM_LEGACY)
> >       unsigned long lock_count; /* DRI1 legacy lock count */

Tvrtko Ursulin May 2, 2023, 7:50 a.m. UTC | #3

On 01/05/2023 17:58, Rob Clark wrote:
> On Fri, Apr 28, 2023 at 4:05 AM Tvrtko Ursulin
> <[e-mail]> wrote:
>>
>>
>> On 27/04/2023 18:53, Rob Clark wrote:
>>> From: Rob Clark <[e-mail]>
>>>
>>> These are useful in particular for VM scenarios where the process which
>>> has opened to drm device file is just a proxy for the real user in a VM
>>> guest.
>>>
>>> Signed-off-by: Rob Clark <[e-mail]>
>>> ---
>>>    Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++
>>>    drivers/gpu/drm/drm_file.c            | 15 +++++++++++++++
>>>    include/drm/drm_file.h                | 19 +++++++++++++++++++
>>>    3 files changed, 52 insertions(+)
>>>
>>> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
>>> index 58dc0d3f8c58..e4877cf8089c 100644
>>> --- a/Documentation/gpu/drm-usage-stats.rst
>>> +++ b/Documentation/gpu/drm-usage-stats.rst
>>> @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` shall be present as well.
>>>    Userspace should make sure to not double account any usage statistics by using
>>>    the above described criteria in order to associate data to individual clients.
>>>
>>> +- drm-comm-override: <valstr>
>>> +
>>> +Returns the client executable override string.  Some drivers support letting
>>> +userspace override this in cases where the userspace is simply a "proxy".
>>> +Such as is the case with virglrenderer drm native context, where the host
>>> +process is just forwarding command submission, etc, from guest userspace.
>>> +This allows the proxy to make visible the executable name of the actual
>>> +app in the VM guest.
>>> +
>>> +- drm-cmdline-override: <valstr>
>>> +
>>> +Returns the client cmdline override string.  Some drivers support letting
>>> +userspace override this in cases where the userspace is simply a "proxy".
>>> +Such as is the case with virglrenderer drm native context, where the host
>>> +process is just forwarding command submission, etc, from guest userspace.
>>> +This allows the proxy to make visible the cmdline of the actual app in the
>>> +VM guest.
>>
>> Perhaps it would be okay to save space here by not repeating the
>> description, like:
>>
>> drm-comm-override: <valstr>
>> drm-cmdline-override: <valstr>
>>
>> Long description blah blah...
>> This allows the proxy to make visible the _executable name *and* command
>> line_ blah blah..
>>
>>> +
>>>    Utilization
>>>    ^^^^^^^^^^^
>>>
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index 9321eb0bf020..d7514c313af1 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>>        spin_lock_init(&file->master_lookup_lock);
>>>        mutex_init(&file->event_read_lock);
>>>
>>> +     mutex_init(&file->override_lock);
>>> +
>>>        if (drm_core_check_feature(dev, DRIVER_GEM))
>>>                drm_gem_open(dev, file);
>>>
>>> @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file)
>>>        WARN_ON(!list_empty(&file->event_list));
>>>
>>>        put_pid(file->pid);
>>> +     kfree(file->override_comm);
>>> +     kfree(file->override_cmdline);
>>>        kfree(file);
>>>    }
>>>
>>> @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f)
>>>                           PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>>        }
>>>
>>> +     mutex_lock(&file->override_lock);
>>
>> You could add a fast unlocked check before taking the mutex for no risk
>> apart a transient false negative. For 99.9999% of userspace it would
>> mean no pointless lock/unlock cycle.
> 
> I'm not sure I get your point?  This needs to be serialized against
> userspace setting the override values

if (file->override_comm || file->override_cmdline) {
	mutex_lock(&file->override_lock);
	if (file->override_comm)
		drm_printf(&p, "drm-comm-override:\t%s\n",
			   file->override_comm);
	if (file->override_cmdline)
		drm_printf(&p, "drm-cmdline-override:\t%s\n",
			   file->override_cmdline);
	mutext_unlock(&file->override_lock);
}

No risk apart for a transient false negative (which is immaterial for 
userspace since fdinfo reads are not ordered versus the override setting 
anyway) and 99.9% of deployments can get by not needing to pointlessly 
cycle the lock.

> 
>>
>>> +     if (file->override_comm) {
>>> +             drm_printf(&p, "drm-comm-override:\t%s\n",
>>> +                        file->override_comm);
>>> +     }
>>> +     if (file->override_cmdline) {
>>> +             drm_printf(&p, "drm-cmdline-override:\t%s\n",
>>> +                        file->override_cmdline);
>>> +     }
>>> +     mutex_unlock(&file->override_lock);
>>> +
>>>        if (dev->driver->show_fdinfo)
>>>                dev->driver->show_fdinfo(&p, file);
>>>    }
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index 1339e925af52..604d05fa6f0c 100644
>>> --- a/include/drm/drm_file.h
>>> +++ b/include/drm/drm_file.h
>>> @@ -370,6 +370,25 @@ struct drm_file {
>>>         */
>>>        struct drm_prime_file_private prime;
>>>
>>> +     /**
>>> +      * @comm: Overridden task comm
>>> +      *
>>> +      * Accessed under override_lock
>>> +      */
>>> +     char *override_comm;
>>> +
>>> +     /**
>>> +      * @cmdline: Overridden task cmdline
>>> +      *
>>> +      * Accessed under override_lock
>>> +      */
>>> +     char *override_cmdline;
>>> +
>>> +     /**
>>> +      * @override_lock: Serialize access to override_comm and override_cmdline
>>> +      */
>>> +     struct mutex override_lock;
>>> +
>>
>> I don't think this should go to drm just yet though. Only one driver can
>> make use of it so I'd leave it for later and print from msm_show_fdinfo
>> for now.
> 
> This was my original approach but danvet asked that it be moved into
> drm for consistency across drivers.  (And really, I want the in-flight
> amd and intel native-context stuff to motivate adding similar features
> to amdgpu/i915/xe.)

IMO if implementation is not shared, not even by using helpers, I don't 
think data storage should be either, but it's not a deal breaker.

Regards,

Tvrtko

> 
> BR,
> -R
> 
>> Regards,
>>
>> Tvrtko
>>
>>>        /* private: */
>>>    #if IS_ENABLED(CONFIG_DRM_LEGACY)
>>>        unsigned long lock_count; /* DRI1 legacy lock count */

Tvrtko Ursulin May 18, 2023, 9:43 a.m. UTC | #4

In case you were waiting for me looking at the rest of the series, there 
was this reply from the previous round I can expand on.

On 02/05/2023 08:50, Tvrtko Ursulin wrote:
> 
> On 01/05/2023 17:58, Rob Clark wrote:
>> On Fri, Apr 28, 2023 at 4:05 AM Tvrtko Ursulin
>> <[e-mail]> wrote:
>>>
>>>
>>> On 27/04/2023 18:53, Rob Clark wrote:
>>>> From: Rob Clark <[e-mail]>
>>>>
>>>> These are useful in particular for VM scenarios where the process which
>>>> has opened to drm device file is just a proxy for the real user in a VM
>>>> guest.
>>>>
>>>> Signed-off-by: Rob Clark <[e-mail]>
>>>> ---
>>>>    Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++
>>>>    drivers/gpu/drm/drm_file.c            | 15 +++++++++++++++
>>>>    include/drm/drm_file.h                | 19 +++++++++++++++++++
>>>>    3 files changed, 52 insertions(+)
>>>>
>>>> diff --git a/Documentation/gpu/drm-usage-stats.rst 
>>>> b/Documentation/gpu/drm-usage-stats.rst
>>>> index 58dc0d3f8c58..e4877cf8089c 100644
>>>> --- a/Documentation/gpu/drm-usage-stats.rst
>>>> +++ b/Documentation/gpu/drm-usage-stats.rst
>>>> @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` 
>>>> shall be present as well.
>>>>    Userspace should make sure to not double account any usage 
>>>> statistics by using
>>>>    the above described criteria in order to associate data to 
>>>> individual clients.
>>>>
>>>> +- drm-comm-override: <valstr>
>>>> +
>>>> +Returns the client executable override string.  Some drivers 
>>>> support letting
>>>> +userspace override this in cases where the userspace is simply a 
>>>> "proxy".
>>>> +Such as is the case with virglrenderer drm native context, where 
>>>> the host
>>>> +process is just forwarding command submission, etc, from guest 
>>>> userspace.
>>>> +This allows the proxy to make visible the executable name of the 
>>>> actual
>>>> +app in the VM guest.
>>>> +
>>>> +- drm-cmdline-override: <valstr>
>>>> +
>>>> +Returns the client cmdline override string.  Some drivers support 
>>>> letting
>>>> +userspace override this in cases where the userspace is simply a 
>>>> "proxy".
>>>> +Such as is the case with virglrenderer drm native context, where 
>>>> the host
>>>> +process is just forwarding command submission, etc, from guest 
>>>> userspace.
>>>> +This allows the proxy to make visible the cmdline of the actual app 
>>>> in the
>>>> +VM guest.
>>>
>>> Perhaps it would be okay to save space here by not repeating the
>>> description, like:
>>>
>>> drm-comm-override: <valstr>
>>> drm-cmdline-override: <valstr>
>>>
>>> Long description blah blah...
>>> This allows the proxy to make visible the _executable name *and* command
>>> line_ blah blah..
>>>
>>>> +
>>>>    Utilization
>>>>    ^^^^^^^^^^^
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>> index 9321eb0bf020..d7514c313af1 100644
>>>> --- a/drivers/gpu/drm/drm_file.c
>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>> @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor 
>>>> *minor)
>>>>        spin_lock_init(&file->master_lookup_lock);
>>>>        mutex_init(&file->event_read_lock);
>>>>
>>>> +     mutex_init(&file->override_lock);
>>>> +
>>>>        if (drm_core_check_feature(dev, DRIVER_GEM))
>>>>                drm_gem_open(dev, file);
>>>>
>>>> @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file)
>>>>        WARN_ON(!list_empty(&file->event_list));
>>>>
>>>>        put_pid(file->pid);
>>>> +     kfree(file->override_comm);
>>>> +     kfree(file->override_cmdline);
>>>>        kfree(file);
>>>>    }
>>>>
>>>> @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct 
>>>> file *f)
>>>>                           PCI_SLOT(pdev->devfn), 
>>>> PCI_FUNC(pdev->devfn));
>>>>        }
>>>>
>>>> +     mutex_lock(&file->override_lock);
>>>
>>> You could add a fast unlocked check before taking the mutex for no risk
>>> apart a transient false negative. For 99.9999% of userspace it would
>>> mean no pointless lock/unlock cycle.
>>
>> I'm not sure I get your point?  This needs to be serialized against
>> userspace setting the override values
> 
> if (file->override_comm || file->override_cmdline) {
>      mutex_lock(&file->override_lock);
>      if (file->override_comm)
>          drm_printf(&p, "drm-comm-override:\t%s\n",
>                 file->override_comm);
>      if (file->override_cmdline)
>          drm_printf(&p, "drm-cmdline-override:\t%s\n",
>                 file->override_cmdline);
>      mutext_unlock(&file->override_lock);
> }
> 
> No risk apart for a transient false negative (which is immaterial for 
> userspace since fdinfo reads are not ordered versus the override setting 
> anyway) and 99.9% of deployments can get by not needing to pointlessly 
> cycle the lock.

This fast path bypass I think is worth it but up to you if you are 
really opposed. It's just that I don't see a point for cycling the mutex 
for nothing in majority of cases.

>>>
>>>> +     if (file->override_comm) {
>>>> +             drm_printf(&p, "drm-comm-override:\t%s\n",
>>>> +                        file->override_comm);
>>>> +     }
>>>> +     if (file->override_cmdline) {
>>>> +             drm_printf(&p, "drm-cmdline-override:\t%s\n",
>>>> +                        file->override_cmdline);
>>>> +     }
>>>> +     mutex_unlock(&file->override_lock);
>>>> +
>>>>        if (dev->driver->show_fdinfo)
>>>>                dev->driver->show_fdinfo(&p, file);
>>>>    }
>>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>>> index 1339e925af52..604d05fa6f0c 100644
>>>> --- a/include/drm/drm_file.h
>>>> +++ b/include/drm/drm_file.h
>>>> @@ -370,6 +370,25 @@ struct drm_file {
>>>>         */
>>>>        struct drm_prime_file_private prime;
>>>>
>>>> +     /**
>>>> +      * @comm: Overridden task comm
>>>> +      *
>>>> +      * Accessed under override_lock
>>>> +      */
>>>> +     char *override_comm;
>>>> +
>>>> +     /**
>>>> +      * @cmdline: Overridden task cmdline
>>>> +      *
>>>> +      * Accessed under override_lock
>>>> +      */
>>>> +     char *override_cmdline;
>>>> +
>>>> +     /**
>>>> +      * @override_lock: Serialize access to override_comm and 
>>>> override_cmdline
>>>> +      */
>>>> +     struct mutex override_lock;
>>>> +
>>>
>>> I don't think this should go to drm just yet though. Only one driver can
>>> make use of it so I'd leave it for later and print from msm_show_fdinfo
>>> for now.
>>
>> This was my original approach but danvet asked that it be moved into
>> drm for consistency across drivers.  (And really, I want the in-flight
>> amd and intel native-context stuff to motivate adding similar features
>> to amdgpu/i915/xe.)
> 
> IMO if implementation is not shared, not even by using helpers, I don't 
> think data storage should be either, but it's not a deal breaker.

To summarise my thoughts on the patch (v4):

I am not really keen on the split of data fields in common and no common 
implementation or helpers.

For what the drm-usage-stats.rst are concerned it looks completely fine. 
And feature really will be useful in virtualised stacks.

Code in this patch is also completely fine.

Therefore you can have an r-b on those parts, but with reservations on 
whether it makes sense to put the fields under drm_file just yet. That 
should be fine under the r-b rules AFAIU. Ideally you can collect an ack 
from someone else too.

Reviewed-by: Tvrtko Ursulin <[e-mail]>

Regards,

Tvrtko

> 
> Regards,
> 
> Tvrtko
> 
>>
>> BR,
>> -R
>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>        /* private: */
>>>>    #if IS_ENABLED(CONFIG_DRM_LEGACY)
>>>>        unsigned long lock_count; /* DRI1 legacy lock count */

Rob Clark May 18, 2023, 4:28 p.m. UTC | #5

On Thu, May 18, 2023 at 2:43 AM Tvrtko Ursulin
<[e-mail]> wrote:
>
>
> In case you were waiting for me looking at the rest of the series, there
> was this reply from the previous round I can expand on.
>
> On 02/05/2023 08:50, Tvrtko Ursulin wrote:
> >
> > On 01/05/2023 17:58, Rob Clark wrote:
> >> On Fri, Apr 28, 2023 at 4:05 AM Tvrtko Ursulin
> >> <[e-mail]> wrote:
> >>>
> >>>
> >>> On 27/04/2023 18:53, Rob Clark wrote:
> >>>> From: Rob Clark <[e-mail]>
> >>>>
> >>>> These are useful in particular for VM scenarios where the process which
> >>>> has opened to drm device file is just a proxy for the real user in a VM
> >>>> guest.
> >>>>
> >>>> Signed-off-by: Rob Clark <[e-mail]>
> >>>> ---
> >>>>    Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++
> >>>>    drivers/gpu/drm/drm_file.c            | 15 +++++++++++++++
> >>>>    include/drm/drm_file.h                | 19 +++++++++++++++++++
> >>>>    3 files changed, 52 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/gpu/drm-usage-stats.rst
> >>>> b/Documentation/gpu/drm-usage-stats.rst
> >>>> index 58dc0d3f8c58..e4877cf8089c 100644
> >>>> --- a/Documentation/gpu/drm-usage-stats.rst
> >>>> +++ b/Documentation/gpu/drm-usage-stats.rst
> >>>> @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev`
> >>>> shall be present as well.
> >>>>    Userspace should make sure to not double account any usage
> >>>> statistics by using
> >>>>    the above described criteria in order to associate data to
> >>>> individual clients.
> >>>>
> >>>> +- drm-comm-override: <valstr>
> >>>> +
> >>>> +Returns the client executable override string.  Some drivers
> >>>> support letting
> >>>> +userspace override this in cases where the userspace is simply a
> >>>> "proxy".
> >>>> +Such as is the case with virglrenderer drm native context, where
> >>>> the host
> >>>> +process is just forwarding command submission, etc, from guest
> >>>> userspace.
> >>>> +This allows the proxy to make visible the executable name of the
> >>>> actual
> >>>> +app in the VM guest.
> >>>> +
> >>>> +- drm-cmdline-override: <valstr>
> >>>> +
> >>>> +Returns the client cmdline override string.  Some drivers support
> >>>> letting
> >>>> +userspace override this in cases where the userspace is simply a
> >>>> "proxy".
> >>>> +Such as is the case with virglrenderer drm native context, where
> >>>> the host
> >>>> +process is just forwarding command submission, etc, from guest
> >>>> userspace.
> >>>> +This allows the proxy to make visible the cmdline of the actual app
> >>>> in the
> >>>> +VM guest.
> >>>
> >>> Perhaps it would be okay to save space here by not repeating the
> >>> description, like:
> >>>
> >>> drm-comm-override: <valstr>
> >>> drm-cmdline-override: <valstr>
> >>>
> >>> Long description blah blah...
> >>> This allows the proxy to make visible the _executable name *and* command
> >>> line_ blah blah..
> >>>
> >>>> +
> >>>>    Utilization
> >>>>    ^^^^^^^^^^^
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> >>>> index 9321eb0bf020..d7514c313af1 100644
> >>>> --- a/drivers/gpu/drm/drm_file.c
> >>>> +++ b/drivers/gpu/drm/drm_file.c
> >>>> @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor
> >>>> *minor)
> >>>>        spin_lock_init(&file->master_lookup_lock);
> >>>>        mutex_init(&file->event_read_lock);
> >>>>
> >>>> +     mutex_init(&file->override_lock);
> >>>> +
> >>>>        if (drm_core_check_feature(dev, DRIVER_GEM))
> >>>>                drm_gem_open(dev, file);
> >>>>
> >>>> @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file)
> >>>>        WARN_ON(!list_empty(&file->event_list));
> >>>>
> >>>>        put_pid(file->pid);
> >>>> +     kfree(file->override_comm);
> >>>> +     kfree(file->override_cmdline);
> >>>>        kfree(file);
> >>>>    }
> >>>>
> >>>> @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct
> >>>> file *f)
> >>>>                           PCI_SLOT(pdev->devfn),
> >>>> PCI_FUNC(pdev->devfn));
> >>>>        }
> >>>>
> >>>> +     mutex_lock(&file->override_lock);
> >>>
> >>> You could add a fast unlocked check before taking the mutex for no risk
> >>> apart a transient false negative. For 99.9999% of userspace it would
> >>> mean no pointless lock/unlock cycle.
> >>
> >> I'm not sure I get your point?  This needs to be serialized against
> >> userspace setting the override values
> >
> > if (file->override_comm || file->override_cmdline) {
> >      mutex_lock(&file->override_lock);
> >      if (file->override_comm)
> >          drm_printf(&p, "drm-comm-override:\t%s\n",
> >                 file->override_comm);
> >      if (file->override_cmdline)
> >          drm_printf(&p, "drm-cmdline-override:\t%s\n",
> >                 file->override_cmdline);
> >      mutext_unlock(&file->override_lock);
> > }
> >
> > No risk apart for a transient false negative (which is immaterial for
> > userspace since fdinfo reads are not ordered versus the override setting
> > anyway) and 99.9% of deployments can get by not needing to pointlessly
> > cycle the lock.
>
> This fast path bypass I think is worth it but up to you if you are
> really opposed. It's just that I don't see a point for cycling the mutex
> for nothing in majority of cases.

I think it is a premature optimization.. an uncontended lock is "just"
an atomic.  Yes, atomics can be expensive in a hot path.. but in this
case it is going to be lost in the noise.  I did look a bit at gputop
with `perf record` and it is very much not the problem.

> >>>
> >>>> +     if (file->override_comm) {
> >>>> +             drm_printf(&p, "drm-comm-override:\t%s\n",
> >>>> +                        file->override_comm);
> >>>> +     }
> >>>> +     if (file->override_cmdline) {
> >>>> +             drm_printf(&p, "drm-cmdline-override:\t%s\n",
> >>>> +                        file->override_cmdline);
> >>>> +     }
> >>>> +     mutex_unlock(&file->override_lock);
> >>>> +
> >>>>        if (dev->driver->show_fdinfo)
> >>>>                dev->driver->show_fdinfo(&p, file);
> >>>>    }
> >>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> >>>> index 1339e925af52..604d05fa6f0c 100644
> >>>> --- a/include/drm/drm_file.h
> >>>> +++ b/include/drm/drm_file.h
> >>>> @@ -370,6 +370,25 @@ struct drm_file {
> >>>>         */
> >>>>        struct drm_prime_file_private prime;
> >>>>
> >>>> +     /**
> >>>> +      * @comm: Overridden task comm
> >>>> +      *
> >>>> +      * Accessed under override_lock
> >>>> +      */
> >>>> +     char *override_comm;
> >>>> +
> >>>> +     /**
> >>>> +      * @cmdline: Overridden task cmdline
> >>>> +      *
> >>>> +      * Accessed under override_lock
> >>>> +      */
> >>>> +     char *override_cmdline;
> >>>> +
> >>>> +     /**
> >>>> +      * @override_lock: Serialize access to override_comm and
> >>>> override_cmdline
> >>>> +      */
> >>>> +     struct mutex override_lock;
> >>>> +
> >>>
> >>> I don't think this should go to drm just yet though. Only one driver can
> >>> make use of it so I'd leave it for later and print from msm_show_fdinfo
> >>> for now.
> >>
> >> This was my original approach but danvet asked that it be moved into
> >> drm for consistency across drivers.  (And really, I want the in-flight
> >> amd and intel native-context stuff to motivate adding similar features
> >> to amdgpu/i915/xe.)
> >
> > IMO if implementation is not shared, not even by using helpers, I don't
> > think data storage should be either, but it's not a deal breaker.
>
> To summarise my thoughts on the patch (v4):
>
> I am not really keen on the split of data fields in common and no common
> implementation or helpers.

I can go either way on this.. it was danvet that suggested moving to
drm_file to encourage more standardization.

(But we can also land the meminfo parts of the series without this
part.. it was just convenient for me to keep them in the same series
to avoid conflicts)

BR,
-R

> For what the drm-usage-stats.rst are concerned it looks completely fine.
> And feature really will be useful in virtualised stacks.
>
> Code in this patch is also completely fine.
>
> Therefore you can have an r-b on those parts, but with reservations on
> whether it makes sense to put the fields under drm_file just yet. That
> should be fine under the r-b rules AFAIU. Ideally you can collect an ack
> from someone else too.
>
> Reviewed-by: Tvrtko Ursulin <[e-mail]>
>
> Regards,
>
> Tvrtko
>
> >
> > Regards,
> >
> > Tvrtko
> >
> >>
> >> BR,
> >> -R
> >>
> >>> Regards,
> >>>
> >>> Tvrtko
> >>>
> >>>>        /* private: */
> >>>>    #if IS_ENABLED(CONFIG_DRM_LEGACY)
> >>>>        unsigned long lock_count; /* DRI1 legacy lock count */

Patch

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index 58dc0d3f8c58..e4877cf8089c 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -73,6 +73,24 @@  scope of each device, in which case `drm-pdev` shall be present as well.
 Userspace should make sure to not double account any usage statistics by using
 the above described criteria in order to associate data to individual clients.
 
+- drm-comm-override: <valstr>
+
+Returns the client executable override string.  Some drivers support letting
+userspace override this in cases where the userspace is simply a "proxy".
+Such as is the case with virglrenderer drm native context, where the host
+process is just forwarding command submission, etc, from guest userspace.
+This allows the proxy to make visible the executable name of the actual
+app in the VM guest.
+
+- drm-cmdline-override: <valstr>
+
+Returns the client cmdline override string.  Some drivers support letting
+userspace override this in cases where the userspace is simply a "proxy".
+Such as is the case with virglrenderer drm native context, where the host
+process is just forwarding command submission, etc, from guest userspace.
+This allows the proxy to make visible the cmdline of the actual app in the
+VM guest.
+
 Utilization
 ^^^^^^^^^^^
 
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 9321eb0bf020..d7514c313af1 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -178,6 +178,8 @@  struct drm_file *drm_file_alloc(struct drm_minor *minor)
 	spin_lock_init(&file->master_lookup_lock);
 	mutex_init(&file->event_read_lock);
 
+	mutex_init(&file->override_lock);
+
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_open(dev, file);
 
@@ -292,6 +294,8 @@  void drm_file_free(struct drm_file *file)
 	WARN_ON(!list_empty(&file->event_list));
 
 	put_pid(file->pid);
+	kfree(file->override_comm);
+	kfree(file->override_cmdline);
 	kfree(file);
 }
 
@@ -995,6 +999,17 @@  void drm_show_fdinfo(struct seq_file *m, struct file *f)
 			   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
 	}
 
+	mutex_lock(&file->override_lock);
+	if (file->override_comm) {
+		drm_printf(&p, "drm-comm-override:\t%s\n",
+			   file->override_comm);
+	}
+	if (file->override_cmdline) {
+		drm_printf(&p, "drm-cmdline-override:\t%s\n",
+			   file->override_cmdline);
+	}
+	mutex_unlock(&file->override_lock);
+
 	if (dev->driver->show_fdinfo)
 		dev->driver->show_fdinfo(&p, file);
 }
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 1339e925af52..604d05fa6f0c 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -370,6 +370,25 @@  struct drm_file {
 	 */
 	struct drm_prime_file_private prime;
 
+	/**
+	 * @comm: Overridden task comm
+	 *
+	 * Accessed under override_lock
+	 */
+	char *override_comm;
+
+	/**
+	 * @cmdline: Overridden task cmdline
+	 *
+	 * Accessed under override_lock
+	 */
+	char *override_cmdline;
+
+	/**
+	 * @override_lock: Serialize access to override_comm and override_cmdline
+	 */
+	struct mutex override_lock;
+
 	/* private: */
 #if IS_ENABLED(CONFIG_DRM_LEGACY)
 	unsigned long lock_count; /* DRI1 legacy lock count */