Re: Proof of concept patch for device rearrangement
Hi, there! On Thursday 19 June 2003 01:25, Poul-Henning Kamp wrote: PK> I have uploaded a proof of concept patch: PK> http://phk.freebsd.dk/patch/fd_dev.patch Just looked thru, not tested yet: * G1/G2 macros could benefit from more descriptive names, e.g. G_LOCK/G_UNLOCK/whatever * the following piece of code in specf_stat() looks like a typo: + sb->st_ino = + sb->st_mode = S_IFCHR | de->de_mode; Are you really going to assign sb->st_ino = sb->st_mode? May it cause syslogd failures you're speaking of? * Why is 'struct devfs_dirent *de' filled with fp->f_data in each and every specf function if it is not used in many of them (e.g. specf_kqfilter, specf_poll, etc) * About XXX comment in the kern_open(): I guess the path is kern_open() -> vn_open() -> vn_open_cred() -[ indirectly thru VOP_OPEN]-> devfs_open(). If it actually is so, then as long as devfs_open() returns error (ENXIO in your case), NDFREE() is performed just before the exit from vn_open_cred() [see the code for the bad: label]. I guess that's the reason why a fictional error is returned, however, returning a 'special' error to indicate a success seems a bit of hackery. I'd suppose to allocate a pseudo-error number and use it instead of ENXIO, just to make clear that actually there's no error. * the wrong comment for DTYPE_DEVICE has been already pointed out by osa@ * The vm/*.c part has some code duplication (the part that considers disablexworkaround, including comment above it). At least, this part could be moved do separate [inline] function. Better yet, the check could be something like 'if (fp->f_type == DTYPE_DEVICE || vp->v_type == VCHR)'. Moreover, am I right that you missed the D_MAP_ANON handling for non-devfs hosted devices (those that reside in a normal FS)? Just to clarify: the old opening scheme is not going to be obsoleted, is it? (I guess it is still necessary for working with special device files not on the devfs) > And with this code enabled, it is possible to go from userland to > device driver without touching Giant underway. AFAICT, neither /dev/null nor /dev/zero have D_NOGIANT at this moment; is it true? And, while you're there: in kern/vfs_syscalls.c in the kern_open() routine, the order of unlocking is not just reverse of their acquirement. Locks are acquired as VOP_LOCK(indirectly via vn_open)->FILEDESC_LOCK->FILE_LOCK, but released as FILEDESC_UNLOCK->FILE_LOCK->VOP_LOCK. This does not pose a deadlock condition as the kern_open is not going to relock FILEDESC_LOCK while holding FILE_LOCK (it is going to drop both), though may be "fixed" from a stylistic point of view. Thank you for your work! Alexey. -- A quoi ca sert d'etre sur la terre Si c'est pour faire nos vies a genoux? ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Proof of concept patch for device rearrangement
On Wed, Jun 18, 2003 at 11:25:36PM +0200, Poul-Henning Kamp wrote: > > I have uploaded a proof of concept patch: > > http://phk.freebsd.dk/patch/fd_dev.patch > > > WARNING: It is perfectly possibly that this patch eats your machine > WARNING: destroys your disk and makes your corn-flakes soggy. > WARNING: RUN AT YOUR OWN RISK! > > > This patch basically shortcuts access from userland to devices > directly from the file descriptor level through DEVFS to the device > driver. > > So far there are no indications that we will not be moving in this > direction pretty soon, but it may not be this exact way we do it, > this is only a proof of concept at this point. > > I have only tested this patch lightly on a disk-less machine, I > have not tried a lot of advanced stuff with it, and I have already > noticed that syslogd seems to have an issue with it during startup > (I just press ctrl-C for now). > > The patch is controlled by the sysctl "debug.phk" which can take > the values 0 (disabled) or 1 (enabled). > > But the good news is that there is a measurable performance improvement: > > syv# sysctl debug.phk=0 > debug.phk: 1 -> 0 > syv# dd if=/dev/zero of=/dev/null count=10 > 10+0 records in > 10+0 records out > 5120 bytes transferred in 4.784862 secs (10700413 bytes/sec) > syv# sysctl debug.phk=1 > debug.phk: 0 -> 1 > syv# dd if=/dev/zero of=/dev/null count=10 > 10+0 records in > 10+0 records out > 5120 bytes transferred in 2.211927 secs (23147238 bytes/sec) > > And with this code enabled, it is possible to go from userland to > device driver without touching Giant underway. > > Comments and test-results are most welcome! Hmm... diff -u -r1.61 file.h --- sys/file.h 18 Jun 2003 19:53:59 - 1.61 +++ sys/file.h 18 Jun 2003 20:44:48 - @@ -62,6 +62,7 @@ #defineDTYPE_FIFO 4 /* fifo (named pipe) */ #defineDTYPE_KQUEUE5 /* event queue */ #defineDTYPE_CRYPTO6 /* crypto */ +#defineDTYPE_DEVICE7 /* crypto */ ^^ looks like comment wrong, isn't it? -- Rgdz,/"\ ASCII RIBBON CAMPAIGN Sergey Osokin aka oZZ, \ /AGAINST HTML MAIL http://ozz.pp.ru/ X AND NEWS / \ ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Proof of concept patch for device rearrangement
I love English! Maxime Henrion wrote: > Giant is the big lock that protects the whole kernel that everyone is > trying to get rid of :-). We should just delete Giant. Then we can get rid of the whole kernel that everyone is trying to get rid of, since Ginat will no longer be protecting it... 8-) 8-). -- Terry ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Proof of concept patch for device rearrangement
On Thu, Jun 19, 2003 at 01:16:56AM +0200, Maxime Henrion wrote: > walt wrote: > > Poul-Henning Kamp wrote: > > > > >I have uploaded a proof of concept patch: > > > > > > http://phk.freebsd.dk/patch/fd_dev.patch > > > > > > >...And with this code enabled, it is possible to go from userland to > > >device driver without touching Giant underway. > > > > I'm sorry, I can't parse that last sentence. Could you explain in > > 25 words or less (or 3 lines of code) what it means? > > > > 'Giant' is the lock that Alan is trying to get rid of? > > Giant is the big lock that protects the whole kernel that everyone is > trying to get rid of :-). Get rid of the lock that is ;-) -- | / o / /_ _ [EMAIL PROTECTED] |/|/ / / /( (_) Bulte ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Proof of concept patch for device rearrangement
walt wrote: > Poul-Henning Kamp wrote: > > >I have uploaded a proof of concept patch: > > > > http://phk.freebsd.dk/patch/fd_dev.patch > > > >...And with this code enabled, it is possible to go from userland to > >device driver without touching Giant underway. > > I'm sorry, I can't parse that last sentence. Could you explain in > 25 words or less (or 3 lines of code) what it means? > > 'Giant' is the lock that Alan is trying to get rid of? Giant is the big lock that protects the whole kernel that everyone is trying to get rid of :-). Maxime ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Proof of concept patch for device rearrangement
Poul-Henning Kamp wrote: I have uploaded a proof of concept patch: http://phk.freebsd.dk/patch/fd_dev.patch ...And with this code enabled, it is possible to go from userland to device driver without touching Giant underway. I'm sorry, I can't parse that last sentence. Could you explain in 25 words or less (or 3 lines of code) what it means? 'Giant' is the lock that Alan is trying to get rid of? Thanks! ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
