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_at_
* 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?