remove BKL from drivers' release functions

remove BKL from drivers' release functions

Post by Rick Lindsle » Sun, 02 Dec 2001 05:20:12



I think you're exactly the right person to be helping out with this;
glad to see you join the discussion.

The biggest flaw in the patch was not realizing that chrdev_open held
the BKL for us during opens as well.  I don't think that invalidates it
in its entirety though.  In cases where the patch replaced BKL with
some other form of locking, we should be no worse off (but for your
concerns about scheduling) because we added it in the open routines
too.  In cases where we removed BKL from release but noted there were
"still" locking issues, yes, we need to examine those closely and fix
them in case we've created a locking issue where none existed before.

In most (all?) cases, the spinlocks are held briefly and (we believe)
not across scheduling opportunities.  If you see areas of the patch
where that is not true, I agree they should be addressed and please point
them out.

Rick
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

remove BKL from drivers' release functions

Post by Alexander Vir » Sun, 02 Dec 2001 08:20:10




>  > ->release() is not serialized AT ALL.  It is serialized for given
>  > struct file, but call open(2) twice and you've got two struct file
>  > for the same device. close() both and you've got two calls of
>  > ->release(), quite possibly - simultaneous.
> OK, that clears some things up.  So, the file->fcount is only used in
> cases where the file descriptor was dup'd, right?

Think for a minute - current IO position is in file->f_pos.  So if two
descriptors have independent current positions (lseek() on one of them
doesn't affect another) they have to have different instances of
struct file behind them.

struct file is an opened channel
descriptor refers to opened channel
file->f_count is the number of references to _this_ channel
dup() creates a descriptor refering to the same channel
so does fork()
open() creates a new channel and descriptor refering to it.

That difference between descriptors and opened channels exists in any
UNIX - otherwise you would either have lseek() done in one program
screwing everybody else or (if fork would create new channels instead
of new references to channels) have (ls; ls) > foo break (shell opens
foo and both ls(1) inherit it over fork(); you _really_ want changes
in current positions resulting from the first ls to affect the
stdout of the second one).

Quote:> back to Alexander  Viro:
>  > In other words, patch is completely bogus.
> No, not completely.  In a lot of cases we just replaced some regular
> arithmetic with atomic instructions of some sort.  These changes are
> still completely valid.  But, in the cases where we added locking, we

Valid, but not necessary a good idea.  Notice that there are very real
races in ->release() and ->open() instances.  Removing BKL from these
(or replacing it with atomic operations) doesn't fix the existing race.
Moreover, it creates a false impression that thing had been reviewed
and fixed.

Quote:> need to reevaluate them for potential problem.  In the cases where we
> just removed the BKL, we really need to check them to make sure that we
> didn't introduce anything.

You really need that in all cases.  Sorry for "told you so", but that's
what I'd been trying to explain from the very beginning - back when the
idea of that patch had been discussed the first time.  That work really
has to be done driver-by-driver...

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

remove BKL from drivers' release functions

Post by Rick Lindsle » Sun, 02 Dec 2001 09:50:14


    > back to Alexander  Viro:
    >  > In other words, patch is completely bogus.
    > No, not completely.  In a lot of cases we just replaced some regular
    > arithmetic with atomic instructions of some sort.  These changes are
    > still completely valid.  But, in the cases where we added locking, we

    Valid, but not necessary a good idea.  Notice that there are very
    real races in ->release() and ->open() instances.  Removing BKL
    from these (or replacing it with atomic operations) doesn't fix the
    existing race.  Moreover, it creates a false impression that thing
    had been reviewed and fixed.

Well, but for the revelation that the BKL is held in chrdev_open(),
offering implicit serialization for all open routines, these *were*
reviewed and thought fixed.  Atomic operations *do* fill the bill when
we are looking at counts used to guarantee exclusive opens.  They
generally *don't* fill the bill when you want to take virtually any
other action based on those counts.

    Sorry for "told you so", but that's what I'd been trying to explain
    from the very beginning - back when the idea of that patch had been
    discussed the first time.  That work really has to be done
    driver-by-driver...

It actually *was* in this case.  This may look like an afternoon of
editor exercise, but each of the cases was inspected and reviewed
before recommending the patch, and collectively represents several
weeks' worth of work.  In early November I posted a note to
linux-kernel which said, in part:

    Before we post these patches, I thought I'd ask if in the time
    since Al Viro moved this out here (July 2000) if anybody
    (especially him!) has found a *legitimate* use of the BKL in the
    release() functions. (We have not found one.)

There was no response.

Given the miss on chrdev_open(), we've got some patch patches to come
up with.  However, this isn't a dump-and-run job.  We accept the
responsibility for those patches, and we'll patch 'em up.  While we
can't then miraculously announce it's safe to remove lock_kernel around
opens, we CAN announce we are closer, and nothing else will be any more
broken than it was before we started. As I said in an earlier post (and
I don't think anybody disagrees): this is an incremental task. Any
assistance you can provide in reviewing this work, including all to
date, is welcome.

Rick
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

remove BKL from drivers' release functions

Post by Rick Lindsle » Sun, 02 Dec 2001 19:10:11


    This is why we have a development tree. Its moving things in the
    right direction which is important. I suspect many drivers will
    want to use semaphores rather than atomic counts however, to ensure
    that an open doesn't complete while a previous release is still
    shutting down hardware

Yes, the only successful application for atomic counts that I've seen
(in this context) is for exclusive open code that looks like

    if (count++) {
        count--;
        return -EBUSY;
    }

in the open routine and

    count--;

in the release.  If you want to do anything else as a result of that
count, you'll need additional locking because it could have changed a
nanosecond after it was (safely) incremented or decremented. You can't
count on it remaining that value after you check it.

release()s that want to shutdown the device, free memory, or take other
actions will want to employ either a spinlock (plain or r/w as
appropriate) or a sleeping semaphore to insure things remain stable
until those actions are complete.

Rick
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

1. Remove needless BKL from release functions

Wether you lock access to shared data at one or zero points doesn't matter,
so it's not breaking it more.

        Christoph

--
Of course it doesn't work. We've performed a software upgrade.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

2. Open Linux Lite won't see NE2000 clone

3. remove BKL from ieee1394_core release function

4. Raids Under Solaris

5. BKL in tiglusb release function

6. 192.168.1.10:80 has no virtual hosts (?)

7. remove BKL from ext2's readdir

8. su to root not anymore working. why is that?

9. remove BKL from inode_setattr

10. drivers/block/block.o: In function `rd_blkdev_pagecache_IO' and `rd_make_request': undefined reference to `bio_size'

11. remove BKL from ext2_get_block() version 2

12. remove BKL from driverfs

13. Remove BKL from NFS read/write code + SunRPC...