The big kernel lock is gone from almost all code in linux-next, this is the status of what I think will happen to the remaining users:
drivers/gpu/drm/i810/{i810,i830}_dma.c: Fixable, but needs someone with the hardware to test. Can probably be marked CONFIG_BROKEN_ON_SMP if nobody cares.
drivers/media/video (V4L): Mauro is working on it, some drivers get moved to staging while the others get fixed. An easy workaround would be possible by adding per-driver mutexes, but Mauro wants to it properly by locking all the right places.
fs/adfs: Probably not hard to fix, but needs someone to test it. adfs has only seen janitorial fixes for the last 5 years. Do we know of any users?
fs/autofs: Pretty much dead, replaced by autofs4. I'd suggest moving this to drivers/staging in 2.6.37 and letting it die there.
fs/coda: Coda seems to have an active community, but not all of their code is actually part of linux (pioctl!), while the last official release is missing many of the cleanups that were don in Linux. Not sure what to do, if someone is interested, the best way might be a fresh start with a merger of the mainline linux and the coda.cs.cmu.edu codebase in drivers/staging. Just removing the BKL without the Coda community seems like a heap of pointless work.
fs/freevxfs: Uses the BKL in readdir and lookup, should be easy to fix. Christoph?
fs/hpfs: Looks fixable, if anyone cares. Maybe it's time for retirement in drivers/staging though. The web page only has a Link to the linux-2.2 version.
fs/lockd: Trond writes that he has someone working on BKL removal here.
fs/locks.c: Patch is under discussion, blocked by work on fs/lockd currently.
fs/ncpfs: Should be fixable if Petr still cares about it. Otherwise suggest moving to drivers/staging if there are no users left.
fs/qnx4: Should be easy to fix, there are only a few places in the code that use the BKL. Anders?
fs/smbfs: Last I heard this was considered obsolete. Should be move it to drivers/staging now?
fs/udf: Not completely trivial, but probably necessary to fix. Project web site is dead, I hope that Jan Kara can be motivated to fix it though.
fs/ufs: Evgeniy Dushistov is maintaining this, I hope he can take care of getting rid of the BKL in UFS.
kernel/trace/blktrace.c: Should be easy. Ingo? Steven?
net/appletalk: net/ipx/af_ipx.c: net/irda/af_irda.c: Can probably be saved from retirement in drivers/staging if the maintainers still care. net/x25: Andrew Hendry has started working on it.
This is all that's left now. I still need to submit a few patches for simple file system changes, but it seems we're getting closer to finally killing it for good.
Arnd
On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
The big kernel lock is gone from almost all code in linux-next, this is the status of what I think will happen to the remaining users:
kernel/trace/blktrace.c: Should be easy. Ingo? Steven?
Jens,
Git blame shows this to be your code (copied from block/blktrace.c from years past).
Is the lock_kernel() needed here? (although Arnd did add it in 62c2a7d9)
-- Steve
On 2010-09-16 16:49, Steven Rostedt wrote:
On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
The big kernel lock is gone from almost all code in linux-next, this is the status of what I think will happen to the remaining users:
kernel/trace/blktrace.c: Should be easy. Ingo? Steven?
Jens,
Git blame shows this to be your code (copied from block/blktrace.c from years past).
Is the lock_kernel() needed here? (although Arnd did add it in 62c2a7d9)
It isn't, it can be removed.
On Thursday 16 September 2010 20:32:36 Jens Axboe wrote:
On 2010-09-16 16:49, Steven Rostedt wrote:
Git blame shows this to be your code (copied from block/blktrace.c from years past).
Is the lock_kernel() needed here? (although Arnd did add it in 62c2a7d9)
It isn't, it can be removed.
Ok, I queued up this patch now. Thanks!
Arnd --- Subject: [PATCH] blktrace: remove the big kernel lock
According to Jens, this code does not need the BKL at all, it is sufficiently serialized by bd_mutex.
Signed-off-by: Arnd Bergmann arnd@arndb.de Cc: Jens Axboe jaxboe@fusionio.com Cc: Steven Rostedt rostedt@goodmis.org
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c index 959f8d6..5328e87 100644 --- a/kernel/trace/blktrace.c +++ b/kernel/trace/blktrace.c @@ -23,7 +23,6 @@ #include <linux/mutex.h> #include <linux/slab.h> #include <linux/debugfs.h> -#include <linux/smp_lock.h> #include <linux/time.h> #include <linux/uaccess.h>
@@ -639,7 +638,6 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg) if (!q) return -ENXIO;
- lock_kernel(); mutex_lock(&bdev->bd_mutex);
switch (cmd) { @@ -667,7 +665,6 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg) }
mutex_unlock(&bdev->bd_mutex); - unlock_kernel(); return ret; }
@@ -1652,10 +1649,9 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev, struct block_device *bdev; ssize_t ret = -ENXIO;
- lock_kernel(); bdev = bdget(part_devt(p)); if (bdev == NULL) - goto out_unlock_kernel; + goto out;
q = blk_trace_get_queue(bdev); if (q == NULL) @@ -1683,8 +1679,7 @@ out_unlock_bdev: mutex_unlock(&bdev->bd_mutex); out_bdput: bdput(bdev); -out_unlock_kernel: - unlock_kernel(); +out: return ret; }
@@ -1714,11 +1709,10 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
ret = -ENXIO;
- lock_kernel(); p = dev_to_part(dev); bdev = bdget(part_devt(p)); if (bdev == NULL) - goto out_unlock_kernel; + goto out;
q = blk_trace_get_queue(bdev); if (q == NULL) @@ -1753,8 +1747,6 @@ out_unlock_bdev: mutex_unlock(&bdev->bd_mutex); out_bdput: bdput(bdev); -out_unlock_kernel: - unlock_kernel(); out: return ret ? ret : count; }
On Thu 16-09-10 16:32:59, Arnd Bergmann wrote:
The big kernel lock is gone from almost all code in linux-next, this is the status of what I think will happen to the remaining users:
...
fs/ncpfs: Should be fixable if Petr still cares about it. Otherwise suggest moving to drivers/staging if there are no users left.
I think some people still use this...
fs/udf: Not completely trivial, but probably necessary to fix. Project web site is dead, I hope that Jan Kara can be motivated to fix it though.
Yeah, I can have a look at it.
Honza
Hi,
On 16 Sep 2010, at 16:04, Jan Kara wrote:
On Thu 16-09-10 16:32:59, Arnd Bergmann wrote:
The big kernel lock is gone from almost all code in linux-next, this is the status of what I think will happen to the remaining users:
...
fs/ncpfs: Should be fixable if Petr still cares about it. Otherwise suggest moving to drivers/staging if there are no users left.
I think some people still use this...
Yes, indeed. Netware is still alive (unfortunately!) and ncpfs is used in a lot of Universities here in the UK at least (we use it about a thousand workstations and servers here at Cambridge University!).
Best regards,
Anton
net/appletalk: net/ipx/af_ipx.c: net/irda/af_irda.c: Can probably be saved from retirement in drivers/staging if the maintainers still care.
IPX and Appletalk both have active users. They also look fairly fixable as the lock_kernel just maps to a stack private mutex, or in several cases can simply be dropped - its just a push down legacy.
IRDA may well be a candidate for staging
From: Alan Cox alan@lxorguk.ukuu.org.uk Date: Thu, 16 Sep 2010 16:07:59 +0100
net/appletalk: net/ipx/af_ipx.c: net/irda/af_irda.c: Can probably be saved from retirement in drivers/staging if the maintainers still care.
IPX and Appletalk both have active users. They also look fairly fixable as the lock_kernel just maps to a stack private mutex, or in several cases can simply be dropped - its just a push down legacy.
I'll take a stab at IPX and Appletalk.
On 2010-09-16 16:32:59, Arnd Bergmann wrote:
The big kernel lock is gone from almost all code in linux-next, this is the status of what I think will happen to the remaining users:
fs/qnx4: Should be easy to fix, there are only a few places in the code that use the BKL. Anders?
Will do.
Cheers Anders
From: Samuel Ortiz samuel@sortiz.org Date: Thu, 16 Sep 2010 18:57:56 +0200
On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
net/appletalk: net/ipx/af_ipx.c: net/irda/af_irda.c: Can probably be saved from retirement in drivers/staging if the maintainers still care.
I'll take care of the IrDA part.
Thanks a lot Sam.
On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
The big kernel lock is gone from almost all code in linux-next, this is the status of what I think will happen to the remaining users:
...
fs/autofs: Pretty much dead, replaced by autofs4. I'd suggest moving this to drivers/staging in 2.6.37 and letting it die there.
Not sure that's what we need. What actually needs to happen is that autofs4 needs to be renamed to autofs.
Ian
This is a update on the current progress for the BKL removal, reflecting what is currently in linux-next.
Maybe we can briefly discuss this at the kernel summit to decide if we want a quick death of the BKL, i.e. fixing/disabling/staging-out the remaining users in 2.6.38 or rather leave them there indefinitely.
On Thursday 16 September 2010, Arnd Bergmann wrote:
The big kernel lock is gone from almost all code in linux-next, this is the status of what I think will happen to the remaining users:
drivers/gpu/drm/i810/{i810,i830}_dma.c: Fixable, but needs someone with the hardware to test. Can probably be marked CONFIG_BROKEN_ON_SMP if nobody cares.
Still open, no good solution for this.
drivers/media/video (V4L): Mauro is working on it, some drivers get moved to staging while the others get fixed. An easy workaround would be possible by adding per-driver mutexes, but Mauro wants to it properly by locking all the right places.
Progressing well, patches are being worked on.
fs/adfs: Probably not hard to fix, but needs someone to test it. adfs has only seen janitorial fixes for the last 5 years. Do we know of any users?
Nobody replied.
fs/autofs: Pretty much dead, replaced by autofs4. I'd suggest moving this to drivers/staging in 2.6.37 and letting it die there.
Now in staging.
fs/coda: Coda seems to have an active community, but not all of their code is actually part of linux (pioctl!), while the last official release is missing many of the cleanups that were don in Linux. Not sure what to do, if someone is interested, the best way might be a fresh start with a merger of the mainline linux and the coda.cs.cmu.edu codebase in drivers/staging. Just removing the BKL without the Coda community seems like a heap of pointless work.
Jan Harkes showed interest, looks like this will get fixed eventually, but probably not in time for 2.6.37.
fs/freevxfs: Uses the BKL in readdir and lookup, should be easy to fix. Christoph?
Still waiting for confirmation from Christoph Hellwig that the BKL is not needed here. I can do the patch to remove it then.
fs/hpfs: Looks fixable, if anyone cares. Maybe it's time for retirement in drivers/staging though. The web page only has a Link to the linux-2.2 version.
No replies.
fs/lockd: Trond writes that he has someone working on BKL removal here.
Bryan Schumaker took care of this, looks like the locking is independent of the fs/locks.c locking now, although it still uses the BKL internally.
I assume that this will get fixed as well, doesn't seem hard. As long as lockd uses the BKL, both nfs and nfsd depend on the BKL implicitly.
fs/locks.c: Patch is under discussion, blocked by work on fs/lockd currently.
No longer blocked now, both lockd and ceph can deal with this converted to spinlocks. I will follow up with the final patch once they hit mainline.
fs/ncpfs: Should be fixable if Petr still cares about it. Otherwise suggest moving to drivers/staging if there are no users left.
Fixed by Petr Vandrovec.
fs/qnx4: Should be easy to fix, there are only a few places in the code that use the BKL. Anders?
Anders Larsen volunteered.
fs/smbfs: Last I heard this was considered obsolete. Should be move it to drivers/staging now?
Now in staging.
fs/udf: Not completely trivial, but probably necessary to fix. Project web site is dead, I hope that Jan Kara can be motivated to fix it though.
Jan Kara volunteered to do it.
fs/ufs: Evgeniy Dushistov is maintaining this, I hope he can take care of getting rid of the BKL in UFS.
No replies.
kernel/trace/blktrace.c: Should be easy. Ingo? Steven?
Done.
net/appletalk: net/ipx/af_ipx.c: net/irda/af_irda.c: Can probably be saved from retirement in drivers/staging if the maintainers still care.
Samuel Ortiz fixed irda.
David Miller volunteered to do appletalk and ipx.
net/x25: Andrew Hendry has started working on it.
Patches have shown up in -next now, I suppose Andrew will finish it soon.
Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end up not getting fixed at all, we can either mark them non-SMP or move them to drivers/staging once all the others are done.
Arnd
Before we get into all these fringe drivers:
- I've not seen any progrss on ->get_sb BKL removal for a while - locks.c is probably a higher priorit, too.
On Monday 18 October 2010 18:19:24 Christoph Hellwig wrote:
Before we get into all these fringe drivers:
- I've not seen any progrss on ->get_sb BKL removal for a while
Not sure what you mean. Jan Blunck did the pushdown into get_sb last year, which is included into linux-next through my bkl/vfs tree. Subsequent patches remove it from most file systems along with the other BKL uses in them. If you like, I can post the series once more, but it has been posted a few times now.
- locks.c is probably a higher priorit, too.
As mentioned in the list, I expect the trivial final patch to be applied in 2.6.37-rc1 after Linus has pulled the trees that this depends on (bkl/vfs, nfs, nfsd, ceph), see below.
This is currently not in -next because of the prerequisites.
Arnd ---
diff --git a/fs/Kconfig b/fs/Kconfig index c386a9f..25ce2dc 100644 --- a/fs/Kconfig +++ b/fs/Kconfig @@ -50,7 +50,6 @@ endif # BLOCK config FILE_LOCKING bool "Enable POSIX file locking API" if EMBEDDED default y - select BKL help This option enables standard file locking support, required for filesystems like NFS and for the flock() system diff --git a/fs/locks.c b/fs/locks.c index 8b2b6ad..02b6e0e 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -142,6 +142,7 @@ int lease_break_time = 45;
static LIST_HEAD(file_lock_list); static LIST_HEAD(blocked_list); +static DEFINE_SPINLOCK(file_lock_lock);
/* * Protects the two list heads above, plus the inode->i_flock list @@ -149,13 +150,13 @@ static LIST_HEAD(blocked_list); */ void lock_flocks(void) { - lock_kernel(); + spin_lock(&file_lock_lock); } EXPORT_SYMBOL_GPL(lock_flocks);
void unlock_flocks(void) { - unlock_kernel(); + spin_unlock(&file_lock_lock); } EXPORT_SYMBOL_GPL(unlock_flocks);
On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end up not getting fixed at all, we can either mark them non-SMP or move them to drivers/staging once all the others are done.
I recommend moving them to staging, and then retire them from there if no one steps up to maintain them.
thanks,
greg k-h
On Tue, Oct 19, 2010 at 4:43 AM, Greg KH greg@kroah.com wrote:
On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end up not getting fixed at all, we can either mark them non-SMP or move them to drivers/staging once all the others are done.
I recommend moving them to staging, and then retire them from there if no one steps up to maintain them.
I think this sets a bad precedent, these drivers work fine. Removing BKL from them is hard, and involves finding and booting hw that developers don't have much time/interest in at the moment. Anyone who has access to the i810 hw and has time to work out the locking has more important things to be doing with modern hw, however it doesn't mean we should just drop support for old drivers because they don't have active maintainers. Removing the BKL from the kernel is a great goal, but breaking userspace ABI by removing drivers isn't.
Dave.
On Tue, Oct 19, 2010 at 09:00:09AM +1000, Dave Airlie wrote:
On Tue, Oct 19, 2010 at 4:43 AM, Greg KH greg@kroah.com wrote:
On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end up not getting fixed at all, we can either mark them non-SMP or move them to drivers/staging once all the others are done.
I recommend moving them to staging, and then retire them from there if no one steps up to maintain them.
I think this sets a bad precedent, these drivers work fine. Removing BKL from them is hard, and involves finding and booting hw that developers don't have much time/interest in at the moment. Anyone who has access to the i810 hw and has time to work out the locking has more important things to be doing with modern hw, however it doesn't mean we should just drop support for old drivers because they don't have active maintainers. Removing the BKL from the kernel is a great goal, but breaking userspace ABI by removing drivers isn't.
Should we just restrict such drivers to only be able to build on UP machines with preempt disabled so that the BKL could be safely removed from them?
Or what other idea do you have as to what could be done here?
I do have access to this hardware, but its on an old single processor laptop, so any work that it would take to help do this development, really wouldn't be able to be tested to be valid at all.
thanks,
greg k-h
On Tue, Oct 19, 2010 at 10:40 AM, Greg KH greg@kroah.com wrote:
On Tue, Oct 19, 2010 at 09:00:09AM +1000, Dave Airlie wrote:
On Tue, Oct 19, 2010 at 4:43 AM, Greg KH greg@kroah.com wrote:
On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end up not getting fixed at all, we can either mark them non-SMP or move them to drivers/staging once all the others are done.
I recommend moving them to staging, and then retire them from there if no one steps up to maintain them.
I think this sets a bad precedent, these drivers work fine. Removing BKL from them is hard, and involves finding and booting hw that developers don't have much time/interest in at the moment. Anyone who has access to the i810 hw and has time to work out the locking has more important things to be doing with modern hw, however it doesn't mean we should just drop support for old drivers because they don't have active maintainers. Removing the BKL from the kernel is a great goal, but breaking userspace ABI by removing drivers isn't.
Should we just restrict such drivers to only be able to build on UP machines with preempt disabled so that the BKL could be safely removed from them?
Or what other idea do you have as to what could be done here?
I do have access to this hardware, but its on an old single processor laptop, so any work that it would take to help do this development, really wouldn't be able to be tested to be valid at all.
There is only very rare case where the i830 driver might get used with SMP and really I think that case is in the don't care place, since if you have that hw you probably should be using i915 on it anyways.
So it really only leaves the problem case of what do distros do if we mark things as BROKEN_ON_SMP, since no distro builds UP kernels and when you boot the SMP kernels on UP they don't run as SMP so not having the driver load on those is a problem. Maybe we just need some sort of warn on smp if a smp unfriendly driver is loaded and we transition to SMP mode. Though this sounds like either (a) something we do now and I don't about it, (b) work.
Dave.
thanks,
greg k-h
On Tue, Oct 19, 2010 at 10:57:43AM +1000, Dave Airlie wrote:
On Tue, Oct 19, 2010 at 10:40 AM, Greg KH greg@kroah.com wrote:
On Tue, Oct 19, 2010 at 09:00:09AM +1000, Dave Airlie wrote:
On Tue, Oct 19, 2010 at 4:43 AM, Greg KH greg@kroah.com wrote:
On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end up not getting fixed at all, we can either mark them non-SMP or move them to drivers/staging once all the others are done.
I recommend moving them to staging, and then retire them from there if no one steps up to maintain them.
I think this sets a bad precedent, these drivers work fine. Removing BKL from them is hard, and involves finding and booting hw that developers don't have much time/interest in at the moment. Anyone who has access to the i810 hw and has time to work out the locking has more important things to be doing with modern hw, however it doesn't mean we should just drop support for old drivers because they don't have active maintainers. Removing the BKL from the kernel is a great goal, but breaking userspace ABI by removing drivers isn't.
Should we just restrict such drivers to only be able to build on UP machines with preempt disabled so that the BKL could be safely removed from them?
Or what other idea do you have as to what could be done here?
I do have access to this hardware, but its on an old single processor laptop, so any work that it would take to help do this development, really wouldn't be able to be tested to be valid at all.
There is only very rare case where the i830 driver might get used with SMP and really I think that case is in the don't care place, since if you have that hw you probably should be using i915 on it anyways.
So, there is no need for the i830 driver? Can it just be removed because i915 works instead?
So it really only leaves the problem case of what do distros do if we mark things as BROKEN_ON_SMP, since no distro builds UP kernels and when you boot the SMP kernels on UP they don't run as SMP so not having the driver load on those is a problem. Maybe we just need some sort of warn on smp if a smp unfriendly driver is loaded and we transition to SMP mode. Though this sounds like either (a) something we do now and I don't about it, (b) work.
So you are saying that just because distros will never build such a thing, we should keep it building for SMP mode? Why not prevent it from being built and if a distro really cares, then they will pony up the development to fix the driver up?
In other words, if someone really cares, then they will do the work, otherwise why worry? Especially as it seems that no one here is going to do it, right?
thanks,
greg k-h
On Tue, Oct 19, 2010 at 12:24 PM, Greg KH greg@kroah.com wrote:
On Tue, Oct 19, 2010 at 10:57:43AM +1000, Dave Airlie wrote:
On Tue, Oct 19, 2010 at 10:40 AM, Greg KH greg@kroah.com wrote:
On Tue, Oct 19, 2010 at 09:00:09AM +1000, Dave Airlie wrote:
On Tue, Oct 19, 2010 at 4:43 AM, Greg KH greg@kroah.com wrote:
On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end up not getting fixed at all, we can either mark them non-SMP or move them to drivers/staging once all the others are done.
I recommend moving them to staging, and then retire them from there if no one steps up to maintain them.
I think this sets a bad precedent, these drivers work fine. Removing BKL from them is hard, and involves finding and booting hw that developers don't have much time/interest in at the moment. Anyone who has access to the i810 hw and has time to work out the locking has more important things to be doing with modern hw, however it doesn't mean we should just drop support for old drivers because they don't have active maintainers. Removing the BKL from the kernel is a great goal, but breaking userspace ABI by removing drivers isn't.
Should we just restrict such drivers to only be able to build on UP machines with preempt disabled so that the BKL could be safely removed from them?
Or what other idea do you have as to what could be done here?
I do have access to this hardware, but its on an old single processor laptop, so any work that it would take to help do this development, really wouldn't be able to be tested to be valid at all.
There is only very rare case where the i830 driver might get used with SMP and really I think that case is in the don't care place, since if you have that hw you probably should be using i915 on it anyways.
So, there is no need for the i830 driver? Can it just be removed because i915 works instead?
No because it provides a different userspace ABI to the i915 driver to a different userspace X driver etc.
like I'm sure the intersection of this driver and reality are getting quite limited, but its still a userspace ABI change and needs to be treated as such. Xorg 6.7 and XFree86 4.3 were the last users of the old driver/API.
So it really only leaves the problem case of what do distros do if we mark things as BROKEN_ON_SMP, since no distro builds UP kernels and when you boot the SMP kernels on UP they don't run as SMP so not having the driver load on those is a problem. Maybe we just need some sort of warn on smp if a smp unfriendly driver is loaded and we transition to SMP mode. Though this sounds like either (a) something we do now and I don't about it, (b) work.
So you are saying that just because distros will never build such a thing, we should keep it building for SMP mode? Why not prevent it from being built and if a distro really cares, then they will pony up the development to fix the driver up?
Distros build the driver now even it it didn't work on SMP it wouldn't matter to the 99% of people who have this hw since it can't suppport SMP except in some corner cases. So not building for SMP is the same as just throwing it out of the kernel since most people don't run kernel.org kernels, and shouldn't have to just to get a driver for some piece of hardware that worked fine up until now.
Look at this from a user who has this hardware pov, it works for them now with a distro kernel, us breaking it isn't going to help that user or make any distro care, its just going to screw over the people who are actually using it.
In other words, if someone really cares, then they will do the work, otherwise why worry? Especially as it seems that no one here is going to do it, right?
Well the thing is doing the work right is a non-trivial task and just dropping support only screws the people using the hardware, it doesn't place any burden on the distro developers to fix it up. If people are really serious about making the BKL go away completely, I think the onus should be on them to fix the drivers not on the users who are using it, like I'm guessing if this gets broken the bug will end up in Novell or RH bugzilla in a year and nobody will ever see it.
Dave.
On Tue, 2010-10-19 at 12:45 +1000, Dave Airlie wrote:
On Tue, Oct 19, 2010 at 12:24 PM, Greg KH greg@kroah.com wrote:
So, there is no need for the i830 driver? Can it just be removed because i915 works instead?
No because it provides a different userspace ABI to the i915 driver to a different userspace X driver etc.
like I'm sure the intersection of this driver and reality are getting quite limited, but its still a userspace ABI change and needs to be treated as such. Xorg 6.7 and XFree86 4.3 were the last users of the old driver/API.
Thus, you are saying that this will break for people with older user apps and have a newer kernel?
So it really only leaves the problem case of what do distros do if we mark things as BROKEN_ON_SMP, since no distro builds UP kernels and when you boot the SMP kernels on UP they don't run as SMP so not having the driver load on those is a problem. Maybe we just need some sort of warn on smp if a smp unfriendly driver is loaded and we transition to SMP mode. Though this sounds like either (a) something we do now and I don't about it, (b) work.
So you are saying that just because distros will never build such a thing, we should keep it building for SMP mode? Why not prevent it from being built and if a distro really cares, then they will pony up the development to fix the driver up?
Distros build the driver now even it it didn't work on SMP it wouldn't matter to the 99% of people who have this hw since it can't suppport SMP except in some corner cases. So not building for SMP is the same as just throwing it out of the kernel since most people don't run kernel.org kernels, and shouldn't have to just to get a driver for some piece of hardware that worked fine up until now.
Ah! Exactly! Thus, those that do not run kernel.org kernels are using a distro kernel. Wont these same people use the distro userspace? That is, if they have upgraded their kernel, most likely, they also update their X interface.
Look at this from a user who has this hardware pov, it works for them now with a distro kernel, us breaking it isn't going to help that user or make any distro care, its just going to screw over the people who are actually using it.
But they can use the i915 driver instead, because they are using the newer userspace apps.
In other words, if someone really cares, then they will do the work, otherwise why worry? Especially as it seems that no one here is going to do it, right?
Well the thing is doing the work right is a non-trivial task and just dropping support only screws the people using the hardware, it doesn't place any burden on the distro developers to fix it up. If people are really serious about making the BKL go away completely, I think the onus should be on them to fix the drivers not on the users who are using it, like I'm guessing if this gets broken the bug will end up in Novell or RH bugzilla in a year and nobody will ever see it.
Well the problem comes down to testing it. I don't know of any developer that is removing the BKL that actually owns hardware to test out these broken drivers. And for the change not being trivial, means that there's no way to do in correctly.
-- Steve
like I'm sure the intersection of this driver and reality are getting quite limited, but its still a userspace ABI change and needs to be treated as such. Xorg 6.7 and XFree86 4.3 were the last users of the old driver/API.
Thus, you are saying that this will break for people with older user apps and have a newer kernel?
There are two drivers here:
i810
i830
The i830 case is the case I care less about since the ABI is only used by older userspace and i915 provides a replacement.
the i810 case ABI is still in use today by distro userspaces that are still released, i.e. i810 is still used in F14, Ubuntu 10.10, RHEL6 Beta etc.
I've snipped the rest of the argument on the grounds you are conflating two cases that aren't the same.
Well the thing is doing the work right is a non-trivial task and just dropping support only screws the people using the hardware, it doesn't place any burden on the distro developers to fix it up. If people are really serious about making the BKL go away completely, I think the onus should be on them to fix the drivers not on the users who are using it, like I'm guessing if this gets broken the bug will end up in Novell or RH bugzilla in a year and nobody will ever see it.
Well the problem comes down to testing it. I don't know of any developer that is removing the BKL that actually owns hardware to test out these broken drivers. And for the change not being trivial, means that there's no way to do in correctly.
So we can drop i830 using deprecation, however its pointless since the fix for i810 is the same fix for i830 if we can work out the fix.
Well the way to do it correctly is make it so if the driver is initialised and we do an SMP transition we warn the users, or we make BROKEN_ON_SMP into a runtime thing that warns when the driver is loaded on an SMP system. The intersection of SMP and this hardware is definitely a very very small number and a lot more workable.
Dave.
On Mon, 18 Oct 2010, Steven Rostedt wrote:
On Tue, 2010-10-19 at 12:45 +1000, Dave Airlie wrote:
On Tue, Oct 19, 2010 at 12:24 PM, Greg KH greg@kroah.com wrote:
So, there is no need for the i830 driver? Can it just be removed because i915 works instead?
No because it provides a different userspace ABI to the i915 driver to a different userspace X driver etc.
like I'm sure the intersection of this driver and reality are getting quite limited, but its still a userspace ABI change and needs to be treated as such. Xorg 6.7 and XFree86 4.3 were the last users of the old driver/API.
Thus, you are saying that this will break for people with older user apps and have a newer kernel?
So it really only leaves the problem case of what do distros do if we mark things as BROKEN_ON_SMP, since no distro builds UP kernels and when you boot the SMP kernels on UP they don't run as SMP so not having the driver load on those is a problem. Maybe we just need some sort of warn on smp if a smp unfriendly driver is loaded and we transition to SMP mode. Though this sounds like either (a) something we do now and I don't about it, (b) work.
So you are saying that just because distros will never build such a thing, we should keep it building for SMP mode? Why not prevent it from being built and if a distro really cares, then they will pony up the development to fix the driver up?
Distros build the driver now even it it didn't work on SMP it wouldn't matter to the 99% of people who have this hw since it can't suppport SMP except in some corner cases. So not building for SMP is the same as just throwing it out of the kernel since most people don't run kernel.org kernels, and shouldn't have to just to get a driver for some piece of hardware that worked fine up until now.
Ah! Exactly! Thus, those that do not run kernel.org kernels are using a distro kernel. Wont these same people use the distro userspace? That is, if they have upgraded their kernel, most likely, they also update their X interface.
Look at this from a user who has this hardware pov, it works for them now with a distro kernel, us breaking it isn't going to help that user or make any distro care, its just going to screw over the people who are actually using it.
But they can use the i915 driver instead, because they are using the newer userspace apps.
In other words, if someone really cares, then they will do the work, otherwise why worry? Especially as it seems that no one here is going to do it, right?
Well the thing is doing the work right is a non-trivial task and just dropping support only screws the people using the hardware, it doesn't place any burden on the distro developers to fix it up. If people are really serious about making the BKL go away completely, I think the onus should be on them to fix the drivers not on the users who are using it, like I'm guessing if this gets broken the bug will end up in Novell or RH bugzilla in a year and nobody will ever see it.
Well the problem comes down to testing it. I don't know of any developer that is removing the BKL that actually owns hardware to test out these broken drivers. And for the change not being trivial, means that there's no way to do in correctly.
-- Steve
I might be able to find some hardware still lying around here that uses an i810. Not sure unless I go hunting it. But I get the impression that if the kernel is a single-CPU kernel there is not any problem anyway? Don't distros offer a non-smp kernel as an installation option in case the user needs it? So in reality how big a problem is this?
Theodore Kilgore
I might be able to find some hardware still lying around here that uses an i810. Not sure unless I go hunting it. But I get the impression that if the kernel is a single-CPU kernel there is not any problem anyway? Don't distros offer a non-smp kernel as an installation option in case the user needs it? So in reality how big a problem is this?
Not anymore, which is my old point of making a fuss. Nowadays in the modern distro world, we supply a single kernel that can at runtime decide if its running on SMP or UP and rewrite the text section appropriately with locks etc. Its like magic, and something like marking drivers as BROKEN_ON_SMP at compile time is really wrong when what you want now is a runtime warning if someone tries to hotplug a CPU with a known iffy driver loaded or if someone tries to load the driver when we are already in SMP mode.
Dave.
On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
I might be able to find some hardware still lying around here that uses an i810. Not sure unless I go hunting it. But I get the impression that if the kernel is a single-CPU kernel there is not any problem anyway? Don't distros offer a non-smp kernel as an installation option in case the user needs it? So in reality how big a problem is this?
Not anymore, which is my old point of making a fuss. Nowadays in the modern distro world, we supply a single kernel that can at runtime decide if its running on SMP or UP and rewrite the text section appropriately with locks etc. Its like magic, and something like marking drivers as BROKEN_ON_SMP at compile time is really wrong when what you want now is a runtime warning if someone tries to hotplug a CPU with a known iffy driver loaded or if someone tries to load the driver when we are already in SMP mode.
We could make the driver run-time non-SMP by adding
if (num_present_cpus() > 1) { pr_err("i810 no longer supports SMP\n"); return -EINVAL; }
to the init function. That would cover the vast majority of the users of i810 hardware, I guess.
Arnd
On Tue, 2010-10-19 at 09:26 +0200, Arnd Bergmann wrote:
On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
I might be able to find some hardware still lying around here that uses an i810. Not sure unless I go hunting it. But I get the impression that if the kernel is a single-CPU kernel there is not any problem anyway? Don't distros offer a non-smp kernel as an installation option in case the user needs it? So in reality how big a problem is this?
Not anymore, which is my old point of making a fuss. Nowadays in the modern distro world, we supply a single kernel that can at runtime decide if its running on SMP or UP and rewrite the text section appropriately with locks etc. Its like magic, and something like marking drivers as BROKEN_ON_SMP at compile time is really wrong when what you want now is a runtime warning if someone tries to hotplug a CPU with a known iffy driver loaded or if someone tries to load the driver when we are already in SMP mode.
We could make the driver run-time non-SMP by adding
if (num_present_cpus() > 1) { pr_err("i810 no longer supports SMP\n"); return -EINVAL; }
to the init function. That would cover the vast majority of the users of i810 hardware, I guess.
I think we also need to cover the PREEMPT case too. But that could be a compile time check, since you can't boot a preempt kernel and make it non preempt.
-- Steve
[trimming Cc list]
On Tuesday 19 October 2010, Steven Rostedt wrote:
I think we also need to cover the PREEMPT case too. But that could be a compile time check, since you can't boot a preempt kernel and make it non preempt.
Right. Can we turn the lock_kernel() into preempt_disable() in these drivers when we know we never run on SMP?
Arnd
On Tue, 2010-10-19 at 15:36 +0200, Arnd Bergmann wrote:
[trimming Cc list]
On Tuesday 19 October 2010, Steven Rostedt wrote:
I think we also need to cover the PREEMPT case too. But that could be a compile time check, since you can't boot a preempt kernel and make it non preempt.
Right. Can we turn the lock_kernel() into preempt_disable() in these drivers when we know we never run on SMP?
I'm not sure that will work. A holder of the BKL can call schedule or even a mutex. The schedule code will drop the BKL and re-enable preemption. Unless the code is known not to schedule while holding BKL, we would need to open code the preempt_enable() around the locations that the code may schedule.
-- Steve
On Tuesday 19 October 2010, Steven Rostedt wrote:
On Tue, 2010-10-19 at 15:36 +0200, Arnd Bergmann wrote:
[trimming Cc list]
On Tuesday 19 October 2010, Steven Rostedt wrote:
I think we also need to cover the PREEMPT case too. But that could be a compile time check, since you can't boot a preempt kernel and make it non preempt.
Right. Can we turn the lock_kernel() into preempt_disable() in these drivers when we know we never run on SMP?
I'm not sure that will work. A holder of the BKL can call schedule or even a mutex. The schedule code will drop the BKL and re-enable preemption. Unless the code is known not to schedule while holding BKL, we would need to open code the preempt_enable() around the locations that the code may schedule.
Right, that won't work then. I was confused by the fact that __lock_kernel() turns into preempt_disable() on UP-preempt systems, which only works because it still maintains current->lock_depth and schedule consequently enables preemption again in __release_kernel_lock(). With CONFIG_BKL disabled, that won't happen any more.
Arnd
On Tue, Oct 19, 2010 at 08:39:58AM -0400, Steven Rostedt wrote:
On Tue, 2010-10-19 at 09:26 +0200, Arnd Bergmann wrote:
On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
I might be able to find some hardware still lying around here that uses an i810. Not sure unless I go hunting it. But I get the impression that if the kernel is a single-CPU kernel there is not any problem anyway? Don't distros offer a non-smp kernel as an installation option in case the user needs it? So in reality how big a problem is this?
Not anymore, which is my old point of making a fuss. Nowadays in the modern distro world, we supply a single kernel that can at runtime decide if its running on SMP or UP and rewrite the text section appropriately with locks etc. Its like magic, and something like marking drivers as BROKEN_ON_SMP at compile time is really wrong when what you want now is a runtime warning if someone tries to hotplug a CPU with a known iffy driver loaded or if someone tries to load the driver when we are already in SMP mode.
We could make the driver run-time non-SMP by adding
if (num_present_cpus() > 1) { pr_err("i810 no longer supports SMP\n"); return -EINVAL; }
to the init function. That would cover the vast majority of the users of i810 hardware, I guess.
I think we also need to cover the PREEMPT case too. But that could be a compile time check, since you can't boot a preempt kernel and make it non preempt.
There are enough nameless embedded vendors that have turned a preempt kernel in to a non-preempt one at run-time by leaking the preempt count, whether by design or not, so it's certainly possile :-)
On Tuesday 19 October 2010, Arnd Bergmann wrote:
On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
I might be able to find some hardware still lying around here that uses an i810. Not sure unless I go hunting it. But I get the impression that if the kernel is a single-CPU kernel there is not any problem anyway? Don't distros offer a non-smp kernel as an installation option in case the user needs it? So in reality how big a problem is this?
Not anymore, which is my old point of making a fuss. Nowadays in the modern distro world, we supply a single kernel that can at runtime decide if its running on SMP or UP and rewrite the text section appropriately with locks etc. Its like magic, and something like marking drivers as BROKEN_ON_SMP at compile time is really wrong when what you want now is a runtime warning if someone tries to hotplug a CPU with a known iffy driver loaded or if someone tries to load the driver when we are already in SMP mode.
We could make the driver run-time non-SMP by adding
if (num_present_cpus() > 1) { pr_err("i810 no longer supports SMP\n"); return -EINVAL; }
to the init function. That would cover the vast majority of the users of i810 hardware, I guess.
Some research showed that Intel never support i810/i815 SMP setups, but there was indeed one company (http://www.acorpusa.com at the time, now owned by a domain squatter) that made i815E based dual Pentium-III boards like this one: http://cgi.ebay.com/280319795096
The first person that can send me an authentic log file showing the use of X.org with DRM on a 2.6.35 kernel with two processors on that mainboard dated today or earlier gets a free upgrade to an AGP graphics card of comparable or better 3D performance from me. Please include the story how why you are running this machine with a new kernel.
i830 is harder, apparently some i865G boards support Pentium 4 with HT and even later dual-core processors.
Arnd
On Tue, Oct 19, 2010 at 11:26 PM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 19 October 2010, Arnd Bergmann wrote:
On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
I might be able to find some hardware still lying around here that uses an i810. Not sure unless I go hunting it. But I get the impression that if the kernel is a single-CPU kernel there is not any problem anyway? Don't distros offer a non-smp kernel as an installation option in case the user needs it? So in reality how big a problem is this?
Not anymore, which is my old point of making a fuss. Nowadays in the modern distro world, we supply a single kernel that can at runtime decide if its running on SMP or UP and rewrite the text section appropriately with locks etc. Its like magic, and something like marking drivers as BROKEN_ON_SMP at compile time is really wrong when what you want now is a runtime warning if someone tries to hotplug a CPU with a known iffy driver loaded or if someone tries to load the driver when we are already in SMP mode.
We could make the driver run-time non-SMP by adding
if (num_present_cpus() > 1) { pr_err("i810 no longer supports SMP\n"); return -EINVAL; }
to the init function. That would cover the vast majority of the users of i810 hardware, I guess.
Some research showed that Intel never support i810/i815 SMP setups, but there was indeed one company (http://www.acorpusa.com at the time, now owned by a domain squatter) that made i815E based dual Pentium-III boards like this one: http://cgi.ebay.com/280319795096
Also that board has no on-board GPU enabled i815EP (P means no on-board GPU).
So I think i810 is fine.
The first person that can send me an authentic log file showing the use of X.org with DRM on a 2.6.35 kernel with two processors on that mainboard dated today or earlier gets a free upgrade to an AGP graphics card of comparable or better 3D performance from me. Please include the story how why you are running this machine with a new kernel.
i830 is harder, apparently some i865G boards support Pentium 4 with HT and even later dual-core processors.
Also hyper-threaded 845G boards, however I'm happy to start a proper deprecation procedure on the i830 ABI, Its been a few years since a distro shipped with it, I think even RHEL5 has the i915 driver enabled, so we are probably talking RHEL4 era.
Dave.
On Wed, Oct 20, 2010 at 06:50:58AM +1000, Dave Airlie wrote:
On Tue, Oct 19, 2010 at 11:26 PM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 19 October 2010, Arnd Bergmann wrote:
On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
I might be able to find some hardware still lying around here that uses an i810. Not sure unless I go hunting it. But I get the impression that if the kernel is a single-CPU kernel there is not any problem anyway? Don't distros offer a non-smp kernel as an installation option in case the user needs it? So in reality how big a problem is this?
Not anymore, which is my old point of making a fuss. Nowadays in the modern distro world, we supply a single kernel that can at runtime decide if its running on SMP or UP and rewrite the text section appropriately with locks etc. Its like magic, and something like marking drivers as BROKEN_ON_SMP at compile time is really wrong when what you want now is a runtime warning if someone tries to hotplug a CPU with a known iffy driver loaded or if someone tries to load the driver when we are already in SMP mode.
We could make the driver run-time non-SMP by adding
if (num_present_cpus() > 1) { pr_err("i810 no longer supports SMP\n"); return -EINVAL; }
to the init function. That would cover the vast majority of the users of i810 hardware, I guess.
Some research showed that Intel never support i810/i815 SMP setups, but there was indeed one company (http://www.acorpusa.com at the time, now owned by a domain squatter) that made i815E based dual Pentium-III boards like this one: http://cgi.ebay.com/280319795096
Also that board has no on-board GPU enabled i815EP (P means no on-board GPU).
A quick search seems to indicate that an i815E variant also existed.
On Mon, 18 Oct 2010 17:40:04 PDT, Greg KH said:
I do have access to this hardware, but its on an old single processor laptop, so any work that it would take to help do this development, really wouldn't be able to be tested to be valid at all.
The i810 is a graphics chipset embedded on the memory controller, which was designed for the Intel Pentium II, Pentium III, and Celeron CPUs. Page 8 of the datasheet specifically says:
Processor/Host Bus Support - Optimized for the Intel Pentium II processor, Intel Pentium III processor, and Intel CeleronTM processor - Supports processor 370-Pin Socket and SC242 connectors - Supports 32-Bit System Bus Addressing - 4 deep in-order queue; 4 or 1 deep request queue - Supports Uni-processor systems only
So no need to clean it up for multiprocessor support.
http://download.intel.com/design/chipsets/datashts/29067602.pdf http://www.intel.com/design/chipsets/specupdt/29069403.pdf
On Tue, Oct 19, 2010 at 02:24:53PM -0400, Valdis.Kletnieks@vt.edu wrote:
On Mon, 18 Oct 2010 17:40:04 PDT, Greg KH said:
I do have access to this hardware, but its on an old single processor laptop, so any work that it would take to help do this development, really wouldn't be able to be tested to be valid at all.
The i810 is a graphics chipset embedded on the memory controller, which was designed for the Intel Pentium II, Pentium III, and Celeron CPUs. Page 8 of the datasheet specifically says:
Processor/Host Bus Support
- Optimized for the Intel Pentium II processor, Intel Pentium III processor, and Intel
CeleronTM processor
- Supports processor 370-Pin Socket and SC242
connectors
- Supports 32-Bit System Bus Addressing
- 4 deep in-order queue; 4 or 1 deep request queue
- Supports Uni-processor systems only
So no need to clean it up for multiprocessor support.
http://download.intel.com/design/chipsets/datashts/29067602.pdf http://www.intel.com/design/chipsets/specupdt/29069403.pdf
Great, we can just drop all calls to lock_kernel() and the like in the driver and be done with it, right?
thanks,
greg k-h
Am Dienstag, 19. Oktober 2010, 21:37:35 schrieb Greg KH:
So no need to clean it up for multiprocessor support.
http://download.intel.com/design/chipsets/datashts/29067602.pdf http://www.intel.com/design/chipsets/specupdt/29069403.pdf
Great, we can just drop all calls to lock_kernel() and the like in the driver and be done with it, right?
No,
you still need to switch off preemption.
Regards Oliver
On Tue, Oct 19, 2010 at 09:40:47PM +0200, Oliver Neukum wrote:
Am Dienstag, 19. Oktober 2010, 21:37:35 schrieb Greg KH:
So no need to clean it up for multiprocessor support.
http://download.intel.com/design/chipsets/datashts/29067602.pdf http://www.intel.com/design/chipsets/specupdt/29069403.pdf
Great, we can just drop all calls to lock_kernel() and the like in the driver and be done with it, right?
No,
you still need to switch off preemption.
Hm, how would you do that from within a driver?
thanks,
greg k-h
On Tue, 19 Oct 2010, Greg KH wrote:
So no need to clean it up for multiprocessor support.
http://download.intel.com/design/chipsets/datashts/29067602.pdf http://www.intel.com/design/chipsets/specupdt/29069403.pdf
Great, we can just drop all calls to lock_kernel() and the like in the driver and be done with it, right?
No,
you still need to switch off preemption.
Hm, how would you do that from within a driver?
preempt_disable()
On Tuesday 19 October 2010 22:41:22 Alan Cox wrote:
you still need to switch off preemption.
Hm, how would you do that from within a driver?
Do we care - unless I misunderstand the current intel DRM driver handles the i810 as well ?
No, it does handle all devices supported by i830.ko (830M, 845M, 854, 855GM, 865G), but not those supported by i810.ko (810, 815).
Arnd
On Tuesday 19 October 2010 22:29:12 Greg KH wrote:
On Tue, Oct 19, 2010 at 09:40:47PM +0200, Oliver Neukum wrote:
Am Dienstag, 19. Oktober 2010, 21:37:35 schrieb Greg KH:
So no need to clean it up for multiprocessor support.
http://download.intel.com/design/chipsets/datashts/29067602.pdf http://www.intel.com/design/chipsets/specupdt/29069403.pdf
Great, we can just drop all calls to lock_kernel() and the like in the driver and be done with it, right?
No,
you still need to switch off preemption.
Hm, how would you do that from within a driver?
I think this would do: --- drm/i810: remove SMP support and BKL
The i810 and i815 chipsets supported by the i810 drm driver were not officially designed for SMP operation, so the big kernel lock is only required for kernel preemption. This disables the driver if preemption is enabled and removes all calls to lock_kernel in it.
If you own an Acorp 6A815EPD mainboard with a i815 chipset and two Pentium-III sockets, and want to run recent kernels on it, tell me about it.
Signed-off-by: Arnd Bergmann arnd@arndb.de ---
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index b755301..e071bc8 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -73,8 +73,8 @@ source "drivers/gpu/drm/radeon/Kconfig"
config DRM_I810 tristate "Intel I810" - # BKL usage in order to avoid AB-BA deadlocks, may become BROKEN_ON_SMP - depends on DRM && AGP && AGP_INTEL && BKL + # PREEMPT requires BKL support here, which was removed + depends on DRM && AGP && AGP_INTEL && !PREEMPT help Choose this option if you have an Intel I810 graphics card. If M is selected, the module will be called i810. AGP support is required diff --git a/drivers/gpu/drm/i810/i810_dma.c b/drivers/gpu/drm/i810/i810_dma.c index ff33e53..8f371e8 100644 --- a/drivers/gpu/drm/i810/i810_dma.c +++ b/drivers/gpu/drm/i810/i810_dma.c @@ -37,7 +37,6 @@ #include <linux/interrupt.h> /* For task queue support */ #include <linux/delay.h> #include <linux/slab.h> -#include <linux/smp_lock.h> #include <linux/pagemap.h>
#define I810_BUF_FREE 2 @@ -94,7 +93,6 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma) struct drm_buf *buf; drm_i810_buf_priv_t *buf_priv;
- lock_kernel(); dev = priv->minor->dev; dev_priv = dev->dev_private; buf = dev_priv->mmap_buffer; @@ -104,7 +102,6 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma) vma->vm_file = filp;
buf_priv->currently_mapped = I810_BUF_MAPPED; - unlock_kernel();
if (io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, @@ -116,7 +113,7 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma) static const struct file_operations i810_buffer_fops = { .open = drm_open, .release = drm_release, - .unlocked_ioctl = i810_ioctl, + .unlocked_ioctl = drm_ioctl, .mmap = i810_mmap_buffers, .fasync = drm_fasync, .llseek = noop_llseek, @@ -1242,19 +1239,6 @@ int i810_driver_dma_quiescent(struct drm_device *dev) return 0; }
-/* - * call the drm_ioctl under the big kernel lock because - * to lock against the i810_mmap_buffers function. - */ -long i810_ioctl(struct file *file, unsigned int cmd, unsigned long arg) -{ - int ret; - lock_kernel(); - ret = drm_ioctl(file, cmd, arg); - unlock_kernel(); - return ret; -} - struct drm_ioctl_desc i810_ioctls[] = { DRM_IOCTL_DEF_DRV(I810_INIT, i810_dma_init, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY|DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(I810_VERTEX, i810_dma_vertex, DRM_AUTH|DRM_UNLOCKED), diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c index 88bcd33..9642d3c 100644 --- a/drivers/gpu/drm/i810/i810_drv.c +++ b/drivers/gpu/drm/i810/i810_drv.c @@ -57,7 +57,7 @@ static struct drm_driver driver = { .owner = THIS_MODULE, .open = drm_open, .release = drm_release, - .unlocked_ioctl = i810_ioctl, + .unlocked_ioctl = drm_ioctl, .mmap = drm_mmap, .poll = drm_poll, .fasync = drm_fasync, @@ -79,6 +79,10 @@ static struct drm_driver driver = {
static int __init i810_init(void) { + if (num_present_cpus() > 1) { + pr_err("drm/i810 does not support SMP\n"); + return -EINVAL; + } driver.num_ioctls = i810_max_ioctl; return drm_init(&driver); }
On Wed, Oct 20, 2010 at 4:44 AM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 19 October 2010 22:29:12 Greg KH wrote:
On Tue, Oct 19, 2010 at 09:40:47PM +0200, Oliver Neukum wrote:
Am Dienstag, 19. Oktober 2010, 21:37:35 schrieb Greg KH:
So no need to clean it up for multiprocessor support.
http://download.intel.com/design/chipsets/datashts/29067602.pdf http://www.intel.com/design/chipsets/specupdt/29069403.pdf
Great, we can just drop all calls to lock_kernel() and the like in the driver and be done with it, right?
No,
you still need to switch off preemption.
Hm, how would you do that from within a driver?
I think this would do:
drm/i810: remove SMP support and BKL
The i810 and i815 chipsets supported by the i810 drm driver were not officially designed for SMP operation, so the big kernel lock is only required for kernel preemption. This disables the driver if preemption is enabled and removes all calls to lock_kernel in it.
If you own an Acorp 6A815EPD mainboard with a i815 chipset and two Pentium-III sockets, and want to run recent kernels on it, tell me about it.
Signed-off-by: Arnd Bergmann arnd@arndb.de
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index b755301..e071bc8 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -73,8 +73,8 @@ source "drivers/gpu/drm/radeon/Kconfig"
config DRM_I810 tristate "Intel I810"
- # BKL usage in order to avoid AB-BA deadlocks, may become BROKEN_ON_SMP
- depends on DRM && AGP && AGP_INTEL && BKL
- # PREEMPT requires BKL support here, which was removed
- depends on DRM && AGP && AGP_INTEL && !PREEMPT
be curious, why can't just fix the lock_kernel logic of i810? Fixing is too hard?
Find a i810 hardware should be possible, even if the hardware does not support SMP, can't we test the fix with preemption?
On Wednesday 20 October 2010, Dave Young wrote:
be curious, why can't just fix the lock_kernel logic of i810? Fixing is too hard?
Find a i810 hardware should be possible, even if the hardware does not support SMP, can't we test the fix with preemption?
Yes, that should work too. My usual approach for removing the BKL without having the hardware myself was to make locking stricter, i.e. replace the BKL with a new spinlock or mutex. This way all the code would still be serialized and if I did something wrong, lockdep would complain about it, but there would be no risk of silent data corruption.
In case of i810, locking across DRM is rather complicated and there is no way of doing this without making changes to other DRM code.
In fact, the only critical section that is actually protected by the BKL are the few lines in i810_mmap_buffers. They look like they might not even need the BKL to start with and we can just remove it even on SMP/PREEMPT, except for perhaps the assignment to buf_priv->currently_mapped. Someone who understands more about the driver than I do can probably figure this out easily, but I couldn't come up with a way that doesn't risk breaking in corner cases.
Arnd
Hi!
@@ -79,6 +79,10 @@ static struct drm_driver driver = {
static int __init i810_init(void) {
- if (num_present_cpus() > 1) {
pr_err("drm/i810 does not support SMP\n");return -EINVAL;- } driver.num_ioctls = i810_max_ioctl; return drm_init(&driver);
Umm, and now someone onlines second cpu?
Pavel
On Tue, Nov 2, 2010 at 3:21 AM, Pavel Machek pavel@ucw.cz wrote:
Hi!
@@ -79,6 +79,10 @@ static struct drm_driver driver = {
static int __init i810_init(void) {
- if (num_present_cpus() > 1) {
- pr_err("drm/i810 does not support SMP\n");
- return -EINVAL;
- }
driver.num_ioctls = i810_max_ioctl; return drm_init(&driver);
Umm, and now someone onlines second cpu?
Well, I see it's being fixed in a different way but yeah, num_possible_cpus() would be more appropriate here.
On Wednesday 03 November 2010, Pekka Enberg wrote:
On Tue, Nov 2, 2010 at 3:21 AM, Pavel Machek pavel@ucw.cz wrote:
Hi!
@@ -79,6 +79,10 @@ static struct drm_driver driver = {
static int __init i810_init(void) {
if (num_present_cpus() > 1) {pr_err("drm/i810 does not support SMP\n");return -EINVAL;} driver.num_ioctls = i810_max_ioctl; return drm_init(&driver);Umm, and now someone onlines second cpu?
Well, I see it's being fixed in a different way but yeah, num_possible_cpus() would be more appropriate here.
(trimming Cc list again)
I thought that patch was still current, what other way are we fixing it now?
Since I'm planning to do one more series for 2.6.37 to take care of the remaining BKL users, should I push the patch above plus the one that marks the module as "depends on !PREEMPT || BROKEN", or should that go through the DRM tree?
Pavel: The only board that has this chipset with multiple sockets is for Pentium III and does not have hotplug sockets, so num_present_cpus is the same as num_possible_cpus here. I can change it to num_possible_cpus of course.
Arnd
On Fri, Nov 5, 2010 at 4:27 AM, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 03 November 2010, Pekka Enberg wrote:
On Tue, Nov 2, 2010 at 3:21 AM, Pavel Machek pavel@ucw.cz wrote:
Hi!
@@ -79,6 +79,10 @@ static struct drm_driver driver = {
static int __init i810_init(void) {
- if (num_present_cpus() > 1) {
- pr_err("drm/i810 does not support SMP\n");
- return -EINVAL;
- }
driver.num_ioctls = i810_max_ioctl; return drm_init(&driver);
Umm, and now someone onlines second cpu?
Well, I see it's being fixed in a different way but yeah, num_possible_cpus() would be more appropriate here.
(trimming Cc list again)
I thought that patch was still current, what other way are we fixing it now?
Oh, I just read the thread wrong. If you repost with num_possible_cpus, feel free to add my ACK.
Any chance you could drop the Cc list a bit for the i810/i830 discussion? It's not relevant to most lists and people Cc'ed here.
dri-devel@lists.freedesktop.org
-
Alan Cox -
Anders Larsen -
Anton Altaparmakov -
Arnd Bergmann -
Christoph Hellwig -
Dave Airlie -
Dave Young -
David Miller -
Greg KH -
Ian Kent -
Jan Kara -
Jens Axboe -
Jiri Kosina -
Oliver Neukum -
Paul Mundt -
Pavel Machek -
Pekka Enberg -
Samuel Ortiz -
Steven Rostedt -
Theodore Kilgore -
Valdis.Kletnieks＠vt.edu -
Ville Syrjälä