From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Herrmann Subject: Re: [PATCH 07/12] drm: drop redundant drm_file->is_master Date: Mon, 28 Jul 2014 17:26:27 +0200 Message-ID: References: <1406129207-1302-1-git-send-email-dh.herrmann@gmail.com> <1406129207-1302-8-git-send-email-dh.herrmann@gmail.com> <20140724095206.GM15237@phenom.ffwll.local> <20140725075641.GM4747@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ie0-f182.google.com (mail-ie0-f182.google.com [209.85.223.182]) by gabe.freedesktop.org (Postfix) with ESMTP id BCC6D6E3A2 for ; Mon, 28 Jul 2014 08:26:27 -0700 (PDT) Received: by mail-ie0-f182.google.com with SMTP id y20so6847511ier.27 for ; Mon, 28 Jul 2014 08:26:27 -0700 (PDT) In-Reply-To: <20140725075641.GM4747@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Daniel Vetter , "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org 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