[PATCH 07/12] drm: drop redundant drm_file->is_master
Hi
On Fri, Jul 25, 2014 at 9:56 AM, Daniel Vetter wrote:
> On Thu, Jul 24, 2014 at 11:38:28PM +0200, David Herrmann wrote:
>> On Thu, Jul 24, 2014 at 11:52 AM, Daniel Vetter wrote:
>> > On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote:
>> >> +static inline bool drm_is_master(const struct drm_file *file)
>> >> +{
>> >
>> > Hm, we don't have any means to synchronize is_master checks with
>> > concurrent ioctls and stuff. Do we care? Orthogonal issue really.
>>
>> We don't.. My drm-master reworks contains a comment about that. It's
>> not really problematic as all ioctls run for a determinate time in
>> kernel-code except for drm_read(), but drm_read() is per-file, not
>> per-device, so we're fine. However, with unfortunate timings, we might
>> really end up with problems.
>>
>> vmwgfx solves this by using separate locks and verifying them against
>> the current master. it's not perfect and I'm not sure I like it better
>> than no locks, but at least they were aware of the problem.
>>
>> Btw., the only thing I know how to solve it properly is to make
>> "master_mutex" an rwlock and all f_op entries take the read-lock,
>> while master modifications take the write-lock. Not sure it's worth
>> it, though.
>
> Imo that's terrible. And I'm not even sure we need to care, e.g. if we do
> a master switch to a different compositor any ioctl the new compositor
> does will grab some locks, which will force the old ioctl to complete.
>
> Well mostly, and only if we don't do lockless tricks or lock-dropping and
> it's racy.
There is always a race! Like this:
CPU A:
1: enter drm_ioctl()
2: check file->is_master
3: enter drm_some_ioctl()
4: acquire some DRM internal locks
If CPU B acquires DRM-Master between step 2 and 3, CPU A might execute
an ioctl with an arbitrary delay. Maybe CPU B even executed some ioctl
after acquiring DRM-Master before CPU A had the chance to enter the
ioctl it's about to execute.
Not that I care much, but we have to remember that those races always
exist. Given that DRM-Master is privileged, this is not really
high-priority to fix..
> I guess a proper fix would be to wait for all ioctls on a device to
> complete. The vfs doesn't have any cool infrastructure for this as part of
> the general revoke work that we could reuse?
What the VFS rework does is this:
if (!atomic_inc_unless_negative(file->sth))
return -ENODEV;
ret = file->f_op->some_op();
atomic_dec(file->sth);
return ret;
That is, it wraps all calls to a file-operation with an
atomic-counter. However, there's only one counter per open-file, not
one per file-operation. If we'd want that for DRM-Master, we couldn't
use it as otherwise all file-operations would be blocked. Furthermore,
VFS only allows disabling an open-file. Once disabled, you cannot
enable it again.
So I don't think a read/write lock is a bad idea. RCU doesn't work as
our ioctls are way to heavy for rcu_read_lock(), SRCU is basically
rw-sem for our use-case. A hand-crafted atomic counter is also
equivalent to rw-sem. So yeah, it might lock nasty, but any other
solution will just hide the fact that we have a read/write lock.
Anyhow, I'm not working on a fix, so if no-one else looks at it, we
can just ignore it.
Thanks
David
[PATCH 07/12] drm: drop redundant drm_file->is_master
On Thu, Jul 24, 2014 at 11:38:28PM +0200, David Herrmann wrote:
> On Thu, Jul 24, 2014 at 11:52 AM, Daniel Vetter wrote:
> > On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote:
> >> +static inline bool drm_is_master(const struct drm_file *file)
> >> +{
> >
> > Hm, we don't have any means to synchronize is_master checks with
> > concurrent ioctls and stuff. Do we care? Orthogonal issue really.
>
> We don't.. My drm-master reworks contains a comment about that. It's
> not really problematic as all ioctls run for a determinate time in
> kernel-code except for drm_read(), but drm_read() is per-file, not
> per-device, so we're fine. However, with unfortunate timings, we might
> really end up with problems.
>
> vmwgfx solves this by using separate locks and verifying them against
> the current master. it's not perfect and I'm not sure I like it better
> than no locks, but at least they were aware of the problem.
>
> Btw., the only thing I know how to solve it properly is to make
> "master_mutex" an rwlock and all f_op entries take the read-lock,
> while master modifications take the write-lock. Not sure it's worth
> it, though.
Imo that's terrible. And I'm not even sure we need to care, e.g. if we do
a master switch to a different compositor any ioctl the new compositor
does will grab some locks, which will force the old ioctl to complete.
Well mostly, and only if we don't do lockless tricks or lock-dropping and
it's racy.
I guess a proper fix would be to wait for all ioctls on a device to
complete. The vfs doesn't have any cool infrastructure for this as part of
the general revoke work that we could reuse?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH 07/12] drm: drop redundant drm_file->is_master
Hi
On Thu, Jul 24, 2014 at 11:52 AM, Daniel Vetter wrote:
> On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote:
>> The drm_file->is_master field is redundant as it's equivalent to:
>> drm_file->master && drm_file->master == drm_file->minor->master
>>
>> 1) "=>"
>> Whenever we set drm_file->is_master, we also set:
>> drm_file->minor->master = drm_file->master;
>>
>> Whenever we clear drm_file->is_master, we also call:
>> drm_master_put(_file->minor->master);
>> which implicitly clears it to NULL.
>>
>> 2) "<="
>> minor->master cannot be set if it is non-NULL. Therefore, it stays as
>> is unless a file drops it.
>>
>> If minor->master is NULL, it is only set by places that also adjust
>> drm_file->is_master.
>>
>> Therefore, we can safely drop is_master and replace it by an inline helper
>> that matches:
>> drm_file->master && drm_file->master == drm_file->minor->master
>>
>> Signed-off-by: David Herrmann
>
> Docbook for drm_is_master is missing, otherwise Reviewed-by: Daniel Vetter
>
>
> And one question below which doesn't really matter for this patch here.
> See below
Fixed, thanks!
> [snip]
>
>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> index d91e09f..e1bb585 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -387,8 +387,6 @@ struct drm_prime_file_private {
>> struct drm_file {
>> unsigned always_authenticated :1;
>> unsigned authenticated :1;
>> - /* Whether we're master for a minor. Protected by master_mutex */
>> - unsigned is_master :1;
>> /* true when the client has asked us to expose stereo 3D mode flags */
>> unsigned stereo_allowed :1;
>> /*
>> @@ -1034,7 +1032,7 @@ struct drm_device {
>> /** \name Locks */
>> /*@{ */
>> struct mutex struct_mutex; /**< For others */
>> - struct mutex master_mutex; /**< For drm_minor::master and
>> drm_file::is_master */
>> + struct mutex master_mutex; /**< For drm_minor::master */
>> /*@} */
>>
>> /** \name Usage Counters */
>> @@ -1172,6 +1170,11 @@ static inline bool drm_is_primary_client(const struct
>> drm_file *file_priv)
>> return file_priv->minor->type == DRM_MINOR_LEGACY;
>> }
>>
>
> Docbook here please ...
>> +static inline bool drm_is_master(const struct drm_file *file)
>> +{
>
> Hm, we don't have any means to synchronize is_master checks with
> concurrent ioctls and stuff. Do we care? Orthogonal issue really.
We don't.. My drm-master reworks contains a comment about that. It's
not really problematic as all ioctls run for a determinate time in
kernel-code except for drm_read(), but drm_read() is per-file, not
per-device, so we're fine. However, with unfortunate timings, we might
really end up with problems.
vmwgfx solves this by using separate locks and verifying them against
the current master. it's not perfect and I'm not sure I like it better
than no locks, but at least they were aware of the problem.
Btw., the only thing I know how to solve it properly is to make
"master_mutex" an rwlock and all f_op entries take the read-lock,
while master modifications take the write-lock. Not sure it's worth
it, though.
Thanks
David
>> + return file->master && file->master == file->minor->master;
>> +}
>> +
>> /**/
>> /** \name Internal function definitions */
>> /*@{*/
>> --
>> 2.0.2
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH 07/12] drm: drop redundant drm_file->is_master
On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote:
> The drm_file->is_master field is redundant as it's equivalent to:
> drm_file->master && drm_file->master == drm_file->minor->master
>
> 1) "=>"
> Whenever we set drm_file->is_master, we also set:
> drm_file->minor->master = drm_file->master;
>
> Whenever we clear drm_file->is_master, we also call:
> drm_master_put(_file->minor->master);
> which implicitly clears it to NULL.
>
> 2) "<="
> minor->master cannot be set if it is non-NULL. Therefore, it stays as
> is unless a file drops it.
>
> If minor->master is NULL, it is only set by places that also adjust
> drm_file->is_master.
>
> Therefore, we can safely drop is_master and replace it by an inline helper
> that matches:
> drm_file->master && drm_file->master == drm_file->minor->master
>
> Signed-off-by: David Herrmann
Docbook for drm_is_master is missing, otherwise Reviewed-by: Daniel Vetter
And one question below which doesn't really matter for this patch here.
See below
-Daniel
[snip]
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index d91e09f..e1bb585 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -387,8 +387,6 @@ struct drm_prime_file_private {
> struct drm_file {
> unsigned always_authenticated :1;
> unsigned authenticated :1;
> - /* Whether we're master for a minor. Protected by master_mutex */
> - unsigned is_master :1;
> /* true when the client has asked us to expose stereo 3D mode flags */
> unsigned stereo_allowed :1;
> /*
> @@ -1034,7 +1032,7 @@ struct drm_device {
> /** \name Locks */
> /*@{ */
> struct mutex struct_mutex; /**< For others */
> - struct mutex master_mutex; /**< For drm_minor::master and
> drm_file::is_master */
> + struct mutex master_mutex; /**< For drm_minor::master */
> /*@} */
>
> /** \name Usage Counters */
> @@ -1172,6 +1170,11 @@ static inline bool drm_is_primary_client(const struct
> drm_file *file_priv)
> return file_priv->minor->type == DRM_MINOR_LEGACY;
> }
>
Docbook here please ...
> +static inline bool drm_is_master(const struct drm_file *file)
> +{
Hm, we don't have any means to synchronize is_master checks with
concurrent ioctls and stuff. Do we care? Orthogonal issue really.
> + return file->master && file->master == file->minor->master;
> +}
> +
> /**/
> /** \name Internal function definitions */
> /*@{*/
> --
> 2.0.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH 07/12] drm: drop redundant drm_file->is_master
The drm_file->is_master field is redundant as it's equivalent to:
drm_file->master && drm_file->master == drm_file->minor->master
1) "=>"
Whenever we set drm_file->is_master, we also set:
drm_file->minor->master = drm_file->master;
Whenever we clear drm_file->is_master, we also call:
drm_master_put(_file->minor->master);
which implicitly clears it to NULL.
2) "<="
minor->master cannot be set if it is non-NULL. Therefore, it stays as
is unless a file drops it.
If minor->master is NULL, it is only set by places that also adjust
drm_file->is_master.
Therefore, we can safely drop is_master and replace it by an inline helper
that matches:
drm_file->master && drm_file->master == drm_file->minor->master
Signed-off-by: David Herrmann
---
drivers/gpu/drm/drm_crtc.c | 2 +-
drivers/gpu/drm/drm_drv.c | 2 +-
drivers/gpu/drm/drm_fops.c | 4 +---
drivers/gpu/drm/drm_lock.c | 2 +-
drivers/gpu/drm/drm_stub.c | 10 +++---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c| 2 +-
include/drm/drmP.h | 9 ++---
8 files changed, 16 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 1ccf5cb..dec0c77 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3188,7 +3188,7 @@ int drm_mode_getfb(struct drm_device *dev,
r->bpp = fb->bits_per_pixel;
r->pitch = fb->pitches[0];
if (fb->funcs->create_handle) {
- if (file_priv->is_master || capable(CAP_SYS_ADMIN) ||
+ if (drm_is_master(file_priv) || capable(CAP_SYS_ADMIN) ||
drm_is_control_client(file_priv)) {
ret = fb->funcs->create_handle(fb, file_priv,
>handle);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 0cc1827..7aa8121 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -307,7 +307,7 @@ static int drm_ioctl_permit(u32 flags, struct drm_file
*file_priv)
return -EACCES;
/* MASTER is only for master or control clients */
- if (unlikely((flags & DRM_MASTER) && !file_priv->is_master &&
+ if (unlikely((flags & DRM_MASTER) && !drm_is_master(file_priv) &&
!drm_is_control_client(file_priv)))
return -EACCES;
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index afba0bf..f65087e 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -286,7 +286,6 @@ static int drm_open_helper(struct file *filp, struct
drm_minor *minor)
goto out_close;
}
- priv->is_master = 1;
/* take another reference for the copy in the local file priv */
priv->master = drm_master_get(priv->minor->master);
priv->authenticated = 1;
@@ -452,7 +451,7 @@ int drm_release(struct inode *inode, struct file *filp)
mutex_lock(>master_mutex);
- if (file_priv->is_master) {
+ if (drm_is_master(file_priv)) {
struct drm_master *master = file_priv->master;
struct drm_file *temp;
@@ -488,7 +487,6 @@ int drm_release(struct inode *inode, struct file *filp)
/* drop the master reference held by the file priv */
if (file_priv->master)
drm_master_put(_priv->master);
- file_priv->is_master = 0;
mutex_unlock(>master_mutex);
mutex_lock(>struct_mutex);
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index f645268..786401c 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -111,7 +111,7 @@ int drm_lock(struct drm_device *dev, void *data, struct
drm_file *file_priv)
/* don't set the block all signals on the master process for now
* really probably not the correct answer but lets us debug xkb
* xserver for now */
- if (!file_priv->is_master) {
+ if (!drm_is_master(file_priv)) {
sigemptyset(>sigmask);
sigaddset(>sigmask, SIGSTOP);
sigaddset(>sigmask, SIGTSTP);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 233ea20..18c9b3d 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -177,7 +177,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
int ret = 0;
mutex_lock(>master_mutex);
- if (file_priv->is_master)
+ if (drm_is_master(file_priv))
goto out_unlock;
if (file_priv->minor->master) {
@@ -191,13 +191,10 @@ int drm_setmaster_ioctl(struct drm_device *dev, void
*data,
}
file_priv->minor->master = drm_master_get(file_priv->master);
- file_priv->is_master = 1;
if
