Re: ioctl(2) vs sys/ioctl.h
On Sat, 18 Dec 2010, der Mouse wrote: > >>int ioctl(int, unsigned long, ...); > > > Most of our ioctl's take pointer arguments. Some streams ioctls > > though take int arguments (ioctl(fd, I_FLUSH, FLUSHR) for example) > > and using void * as the argument would not compile cleanly. > > Must FLUSHR (to continue your example) be defined as an int value? it seems to be a flag > I'm just brainstorming possible ways to avoid inflicting a varargs > declaration on all users of ioctl. they already have it, see and http://pubs.opengroup.org/onlinepubs/009695399/functions/ioctl.html iain
locking around LFS_{SET,CLR}_UINO
Hello LFS developers,
When vmlocking2 was merged, lock and unlock were moved out of
LFS_{SET,CLR}_UINO.
: % diff -U4 lfs.h:1.{122,123} | sed -n 94,104p
: #define LFS_SET_UINO(ip, flags) do { \
: - simple_lock(&(ip)->i_lfs->lfs_interlock); \
: if (((flags) & IN_ACCESSED) && !((ip)->i_flag & IN_ACCESSED)) \
: ++(ip)->i_lfs->lfs_uinodes; \
: if (((flags) & IN_CLEANING) && !((ip)->i_flag & IN_CLEANING)) \
: ++(ip)->i_lfs->lfs_uinodes; \
: if (((flags) & IN_MODIFIED) && !((ip)->i_flag & IN_MODIFIED)) \
: ++(ip)->i_lfs->lfs_uinodes; \
: (ip)->i_flag |= (flags);\
: - simple_unlock(&(ip)->i_lfs->lfs_interlock); \
: } while (0)
I guess the lock protects lfs_uinodes, when IN_ACCESSED, IN_CLEANING
or IN_MODIFIED are set.
If it is right, a lock is unnecessary and some locks are missing. See
attached patch.
Cheers,
--
nakayosh
Date: Sun Dec 19 23:24:53 JST 2010
diff -u src/sys/ufs/lfs/lfs_alloc.c.ORIG src/sys/ufs/lfs/lfs_alloc.c
--- src/sys/ufs/lfs/lfs_alloc.c.ORIG2010-06-24 22:03:19.0 +0900
+++ src/sys/ufs/lfs/lfs_alloc.c 2010-12-19 23:24:27.0 +0900
@@ -1,4 +1,4 @@
-/* $NetBSD: lfs_alloc.c,v 1.110 2010/06/24 13:03:19 hannken Exp $ */
+/* $NetBSD$*/
/*-
* Copyright (c) 1999, 2000, 2001, 2002, 2003, 2007 The NetBSD Foundation, Inc.
@@ -276,9 +276,7 @@
lfs_vcreate(pvp->v_mount, new_ino, vp);
ip = VTOI(vp);
- mutex_enter(&lfs_lock);
LFS_SET_UINO(ip, IN_CHANGE);
- mutex_exit(&lfs_lock);
/* on-disk structure has been zeroed out by lfs_vcreate */
ip->i_din.ffs1_din->di_inumber = new_ino;
@@ -477,9 +475,8 @@
}
mutex_enter(&lfs_lock);
- LFS_CLR_UINO(ip, IN_ACCESSED|IN_CLEANING|IN_MODIFIED);
+ LFS_CLR_UINO(ip, IN_ALLMOD);
mutex_exit(&lfs_lock);
- ip->i_flag &= ~IN_ALLMOD;
ip->i_lfs_iflags |= LFSI_DELETED;
/*
diff -u src/sys/ufs/lfs/lfs_segment.c.ORIG src/sys/ufs/lfs/lfs_segment.c
--- src/sys/ufs/lfs/lfs_segment.c.ORIG 2010-07-22 02:52:13.0 +0900
+++ src/sys/ufs/lfs/lfs_segment.c 2010-12-19 23:24:39.0 +0900
@@ -1,4 +1,4 @@
-/* $NetBSD: lfs_segment.c,v 1.217 2010/07/21 17:52:13 hannken Exp $
*/
+/* $NetBSD$*/
/*-
* Copyright (c) 1999, 2000, 2001, 2002, 2003 The NetBSD Foundation, Inc.
@@ -324,9 +324,9 @@
}
}
mutex_exit(&bufcache_lock);
- LFS_CLR_UINO(ip, IN_CLEANING);
- LFS_CLR_UINO(ip, IN_MODIFIED | IN_ACCESSED);
- ip->i_flag &= ~IN_ALLMOD;
+ mutex_enter(&lfs_lock);
+ LFS_CLR_UINO(ip, IN_ALLMOD);
+ mutex_exit(&lfs_lock);
DLOG((DLOG_VNODE, "lfs_vflush: done not flushing ino %d\n",
ip->i_number));
lfs_segunlock(fs);
@@ -767,7 +767,9 @@
*/
mutex_enter(&vp->v_interlock);
if (LIST_EMPTY(&vp->v_dirtyblkhd)) {
+ mutex_enter(&lfs_lock);
LFS_CLR_UINO(ip, IN_ALLMOD);
+ mutex_exit(&lfs_lock);
}
#ifdef DIAGNOSTIC
else if (do_ckp) {
@@ -1219,6 +1221,7 @@
}
#endif /* DIAGNOSTIC */
+ mutex_enter(&lfs_lock);
if (ip->i_flag & IN_CLEANING)
LFS_CLR_UINO(ip, IN_CLEANING);
else {
@@ -1233,6 +1236,7 @@
ip->i_ffs1_blocks, ip->i_lfs_effnblks));
}
}
+ mutex_exit(&lfs_lock);
if (ip->i_number == LFS_IFILE_INUM) {
/* We know sp->idp == NULL */
@@ -1665,7 +1669,9 @@
}
/* This inode has been modified */
+ mutex_enter(&lfs_lock);
LFS_SET_UINO(VTOI(vp), IN_MODIFIED);
+ mutex_exit(&lfs_lock);
}
/*
diff -u src/sys/ufs/lfs/lfs_vnops.c.ORIG src/sys/ufs/lfs/lfs_vnops.c
--- src/sys/ufs/lfs/lfs_vnops.c.ORIG2010-12-18 09:01:46.0 +0900
+++ src/sys/ufs/lfs/lfs_vnops.c 2010-12-19 23:24:49.0 +0900
@@ -1,4 +1,4 @@
-/* $NetBSD: lfs_vnops.c,v 1.232 2010/12/18 00:01:46 eeh Exp $ */
+/* $NetBSD$*/
/*-
* Copyright (c) 1999, 2000, 2001, 2002, 2003 The NetBSD Foundation, Inc.
@@ -332,8 +332,11 @@
error = VOP_IOCTL(ip->i_devvp, DIOCCACHESYNC, &l, FWRITE,
curlwp->l_cred);
}
- if (wait && !VPISEMPTY(vp))
+ if (wait && !VPISEMPTY(vp)) {
+ mutex_enter(&lfs_lock);
LFS_SET_UINO(ip, IN_MODIFIED);
+ mutex_exit(&lfs_lock);
+ }
return error;
}
Re: ioctl(2) vs sys/ioctl.h
On Sun, 19 Dec 2010, Christos Zoulas wrote: In article , Paul Goyette wrote: Is there some reason why there is a discrepancy in the definition of ioctl()? From man page ioctl(2) SYNOPSIS #include int ioctl(int d, unsigned long request, void *argp); Yet, from sys/ioctl.h we have __BEGIN_DECLS int ioctl(int, unsigned long, ...); __END_DECLS Most of our ioctl's take pointer arguments. Some streams ioctls though take int arguments (ioctl(fd, I_FLUSH, FLUSHR) for example) and using void * as the argument would not compile cleanly. I think that we should not have void * in the man page either. Should the man page be updated to match reality? - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: ioctl(2) vs sys/ioctl.h
On Dec 19, 8:00am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: ioctl(2) vs sys/ioctl.h | Should the man page be updated to match reality? I just did. christos
Re: ioctl(2) vs sys/ioctl.h
On Sat, Dec 18, 2010 at 04:06:15PM -0800, Paul Goyette wrote: > Is there some reason why there is a discrepancy in the definition of > ioctl()? > > >From man page ioctl(2) ... >ioctl(int d, unsigned long request, void *argp); > Yet, from sys/ioctl.h we have ... > int ioctl(int, unsigned long, ...); As mentioned by others, historically the 3rd arg to ioctl was either a pointer or a constant. Pre ANSI-C this didn't matter. There is a bigger problem, the 'int' and 'void *' arguments might be passed in different ways then '...' is specified. For a userspace varargs function this isn't a problem - it is taken care of by va_arg(), but for a sycall stub this isn't true since the type of the argument isn't known early enough. I suspect the only form that will work is soemthing like: int ioctl(int, unsigned long, void *); #define ioctl(fd, cmd, arg) ioctl(fd, cmd, (void *)(intptr_t)(arg)) We only get away with it on our 64 bit archs because they all pass the first 3 arguments in registers. David -- David Laight: da...@l8s.co.uk
freebsd 5.99.41 as XEN3_DOMU
I am having a bit of trouble given that I am not super familiar with NetBSD (although I hope that will change). Just compiled netbsd-current in XEN domain installed userland and all the new kernel modules in the DOMU root file system, booted the new kernel and everything works great except for a problem that should be obvious but I can't figure it out... when I issue a modload solaris (or for that matter a modload "anything") I get the following message: modload: Function not implemented What am I doing wrong? Probably an obvious answer. The whole reason I want to run current is to test ZFS on NetBSD. Thanks RB
Re: ioctl(2) vs sys/ioctl.h
On Sun 19 Dec 2010 at 19:32:49 +, David Laight wrote: > I suspect the only form that will work is soemthing like: > > int ioctl(int, unsigned long, void *); > #define ioctl(fd, cmd, arg) ioctl(fd, cmd, (void *)(intptr_t)(arg)) Easier: the aforementioned constant FLUSHR (and all others) can be defined as ((void *)1234) (for appropriate values of 1234). However, we don't have streams, so no streams ioctls, which makes the point moot, at least for the given example. > David -Olaf. -- ___ Olaf 'Rhialto' Seibert -- There's no point being grown-up if you \X/ rhialto/at/xs4all.nl-- can't be childish sometimes. -The 4th Doctor
Re: ioctl(2) vs sys/ioctl.h
On Sun, Dec 19, 2010 at 09:06:31PM +0100, Rhialto wrote: > > Easier: the aforementioned constant FLUSHR (and all others) can be > defined as ((void *)1234) (for appropriate values of 1234). FLUSHR (and FLUSHW) have to be numbers - they are bit patterns and can be or'ed together. > However, we don't have streams, so no streams ioctls, which makes the > point moot, at least for the given example. Historically (and outside netbsd) integer args are more common (and the 'cmd' can be 32 bits). David -- David Laight: da...@l8s.co.uk
Re: freebsd 5.99.41 as XEN3_DOMU
On Sun, Dec 19, 2010 at 02:35:08PM -0500, Robert Boyer wrote: > I am having a bit of trouble given that I am not super familiar with NetBSD > (although I hope that will change). > > Just compiled netbsd-current in XEN domain installed userland and all the new > kernel modules in the DOMU root file system, booted the new kernel and > everything works great except for a problem that should be obvious but I > can't figure it out... > > when I issue a modload solaris (or for that matter a modload "anything") I > get the following message: > > modload: Function not implemented > > What am I doing wrong? Probably an obvious answer. > > The whole reason I want to run current is to test ZFS on NetBSD. Well, in the current state, modules are a not enabled in the Xen kernels (modules should be built specifically for Xen, but the build tools do not allow this right now). So you have to compile all what you need in a monolitic kernel. But ZFS is only available as module, so unfortunably this means no ZFS for xen. One way around it is to run NetBSD in a HVM guest. -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: freebsd 5.99.41 as XEN3_DOMU
Manuel Bouyer writes: > Well, in the current state, modules are a not enabled in the Xen kernels > (modules should be built specifically for Xen, but the build tools do not > allow this right now). So you have to compile all what you need in a > monolitic kernel. But ZFS is only available as module, so unfortunably > this means no ZFS for xen. > One way around it is to run NetBSD in a HVM guest. Is it that in src/share/mk/bsd.kmodule.mk one would have to add some support to build a XEN DOMU flavor of modules?Is it reasonably easy to just hack it in so that one gets XEN modules *instead* of standard modules? (Obviously not suitable for committing, but could be useful.) pgpKZHDybj1Mp.pgp Description: PGP signature
Re: ioctl(2) vs sys/ioctl.h
> There is a bigger problem, the 'int' and 'void *' arguments might be > passed in different ways then '...' is specified. True, but it is not inherently a problem; it just complicates the implementation of ioctl(), since it then has to not just pass down a data pointer, but pass down enough information for the particular ioctl's implementation to find whatever type the actual argument is. (As it is, the implementation already depends on a nonportability, basically that all pointer types are "the same". It would explode badly on a machine where some but not all pointer types are larger than a machine word/register.) > We only get away with it on our 64 bit archs because they all pass > the first 3 arguments in registers. ...and are all byte-addressed. If some pointers were 64 bits and others were 128 (or, worse, 72 or 96 or some such), it would fall over rather hard. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: ioctl(2) vs sys/ioctl.h
In article <20101219200631.gc14...@falu.nl>, Rhialto wrote: >On Sun 19 Dec 2010 at 19:32:49 +, David Laight wrote: >> I suspect the only form that will work is soemthing like: >> >> int ioctl(int, unsigned long, void *); >> #define ioctl(fd, cmd, arg) ioctl(fd, cmd, (void *)(intptr_t)(arg)) > >Easier: the aforementioned constant FLUSHR (and all others) can be >defined as ((void *)1234) (for appropriate values of 1234). > >However, we don't have streams, so no streams ioctls, which makes the >point moot, at least for the given example. Ok, how about *all* the examples in ioctl(2)? :-) christos
Re: locking around LFS_{SET,CLR}_UINO
On Sun, 19 Dec 2010, NAKAJIMA Yoshihiro wrote:
> Hello LFS developers,
>
>
> When vmlocking2 was merged, lock and unlock were moved out of
> LFS_{SET,CLR}_UINO.
>
> : % diff -U4 lfs.h:1.{122,123} | sed -n 94,104p
> : #define LFS_SET_UINO(ip, flags) do {
> \
> : - simple_lock(&(ip)->i_lfs->lfs_interlock); \
> : if (((flags) & IN_ACCESSED) && !((ip)->i_flag & IN_ACCESSED)) \
> : ++(ip)->i_lfs->lfs_uinodes; \
> : if (((flags) & IN_CLEANING) && !((ip)->i_flag & IN_CLEANING)) \
> : ++(ip)->i_lfs->lfs_uinodes; \
> : if (((flags) & IN_MODIFIED) && !((ip)->i_flag & IN_MODIFIED)) \
> : ++(ip)->i_lfs->lfs_uinodes; \
> : (ip)->i_flag |= (flags);\
> : - simple_unlock(&(ip)->i_lfs->lfs_interlock); \
> : } while (0)
>
> I guess the lock protects lfs_uinodes, when IN_ACCESSED, IN_CLEANING
> or IN_MODIFIED are set.
>
> If it is right, a lock is unnecessary and some locks are missing. See
> attached patch.
Looks reasonable. You should definitely add a comment somewhere
indicating the uino is protected by the lfs_lock. Locking protocols must
be documented or they are guaranteed to be broken. Have you tested it
under load?
Eduardo
re: ioctl(2) vs sys/ioctl.h
> On Sun 19 Dec 2010 at 19:32:49 +, David Laight wrote: > > I suspect the only form that will work is soemthing like: > > > > int ioctl(int, unsigned long, void *); > > #define ioctl(fd, cmd, arg) ioctl(fd, cmd, (void *)(intptr_t)(arg)) > > Easier: the aforementioned constant FLUSHR (and all others) can be > defined as ((void *)1234) (for appropriate values of 1234). > > However, we don't have streams, so no streams ioctls, which makes the > point moot, at least for the given example. this changes the ABI on LP64BE if i am not mistaken. .mrg.
Re: ioctl(2) vs sys/ioctl.h
In article <20474.1292802...@splode.eterna.com.au>, matthew green wrote: > >> On Sun 19 Dec 2010 at 19:32:49 +, David Laight wrote: >> > I suspect the only form that will work is soemthing like: >> > >> > int ioctl(int, unsigned long, void *); >> > #define ioctl(fd, cmd, arg) ioctl(fd, cmd, (void *)(intptr_t)(arg)) >> >> Easier: the aforementioned constant FLUSHR (and all others) can be >> defined as ((void *)1234) (for appropriate values of 1234). >> >> However, we don't have streams, so no streams ioctls, which makes the >> point moot, at least for the given example. > >this changes the ABI on LP64BE if i am not mistaken. That would change the ABI on _LP64 systems that don't pass the first 3 arguments in registers. We don't have any yet. On the other hand we are not planning to change ioctl like this for no good reason (and we would break standards compliance). I just changed the man pages to be consistent with the implementation. christos
Consistent WARN_REFERENCES for assembler
Hi all, unless there are objections, I am going to commit the attached patch soon. It replaces the use of stabs for WARN_REFERENCES in assembler code with the modernish .gnu.warning sections. Platforms that already used this now consistently use .pushsection / .popsection. Joerg Index: alpha/include/asm.h === RCS file: /home/joerg/repo/netbsd/src/sys/arch/alpha/include/asm.h,v retrieving revision 1.30 diff -u -p -r1.30 asm.h --- alpha/include/asm.h 9 Feb 2007 21:55:01 - 1.30 +++ alpha/include/asm.h 19 Dec 2010 20:23:23 - @@ -628,11 +628,15 @@ label: ASCIZ msg; \ * WARN_REFERENCES: create a warning if the specified symbol is referenced. */ #ifdef __STDC__ -#define WARN_REFERENCES(_sym,_msg)\ - .section .gnu.warning. ## _sym ; .ascii _msg ; .text +#define WARN_REFERENCES(sym,msg) \ + .pushsection .gnu.warning. ## sym;\ + .ascii msg; \ + .popsection #else -#define WARN_REFERENCES(_sym,_msg)\ - .section .gnu.warning./**/_sym ; .ascii _msg ; .text +#define WARN_REFERENCES(sym,msg) \ + .pushsection .gnu.warning./**/sym;\ + .ascii msg; \ + .popsection #endif /* __STDC__ */ /* Index: amd64/include/asm.h === RCS file: /home/joerg/repo/netbsd/src/sys/arch/amd64/include/asm.h,v retrieving revision 1.13 diff -u -p -r1.13 asm.h --- amd64/include/asm.h 26 Oct 2008 00:08:15 - 1.13 +++ amd64/include/asm.h 19 Dec 2010 20:18:09 - @@ -119,15 +119,16 @@ .globl alias; \ alias = sym -/* XXXfvdl do not use stabs here */ #ifdef __STDC__ #define WARN_REFERENCES(sym,msg) \ - .stabs msg ## ,30,0,0,0 ; \ - .stabs __STRING(_C_LABEL(sym)) ## ,1,0,0,0 + .pushsection .gnu.warning. ## sym;\ + .ascii msg; \ + .popsection #else #define WARN_REFERENCES(sym,msg) \ - .stabs msg,30,0,0,0 ; \ - .stabs __STRING(sym),1,0,0,0 + .pushsection .gnu.warning./**/sym;\ + .ascii msg; \ + .popsection #endif /* __STDC__ */ #else /* __x86_64__ */ Index: arm/include/asm.h === RCS file: /home/joerg/repo/netbsd/src/sys/arch/arm/include/asm.h,v retrieving revision 1.12 diff -u -p -r1.12 asm.h --- arm/include/asm.h 29 Aug 2008 19:00:25 - 1.12 +++ arm/include/asm.h 19 Dec 2010 20:21:22 - @@ -139,9 +139,17 @@ .globl alias; \ alias = sym +#ifdef __STDC__ #define WARN_REFERENCES(sym,msg) \ - .stabs msg,30,0,0,0 ; \ - .stabs __STRING(_C_LABEL(sym)),1,0,0,0 + .pushsection .gnu.warning. ## sym;\ + .ascii msg; \ + .popsection +#else +#define WARN_REFERENCES(sym,msg) \ + .pushsection .gnu.warning./**/sym;\ + .ascii msg; \ + .popsection +#endif /* __STDC__ */ #ifdef __thumb__ # define XPUSH push Index: hppa/include/asm.h === RCS file: /home/joerg/repo/netbsd/src/sys/arch/hppa/include/asm.h,v retrieving revision 1.11 diff -u -p -r1.11 asm.h --- hppa/include/asm.h 8 Mar 2010 07:42:46 - 1.11 +++ hppa/include/asm.h 19 Dec 2010 20:17:18 - @@ -135,13 +135,15 @@ #ifdef __STDC__ #define WARN_REFERENCES(sym,msg) \ - .stabs msg ## ,30,0,0,0 ; \ - .stabs __STRING(sym) ## ,1,0,0,0 + .pushsection .gnu.warning. ## sym;\ + .ascii msg; \ + .popsection #else #define WARN_REFERENCES(sym,msg) \ - .stabs msg,30,0,0,0 ; \ - .stabs __STRING(sym),1,0,0,0 -#endif + .pushsection .gnu.warning./**/sym;\ + .ascii msg; \ + .popsection +#endif /* __STDC__ */ #define BSS(n,s) .comm n, s #define SZREG 4 Index: i386/include/asm.h === RCS file: /home/joerg/repo/netbsd/src/sys/arch/i386/include/asm.h,v retrieving revision 1.38 diff -u -p -r1.38 asm.h --- i386/include/asm.h 3 May 2008 05:54:52 - 1.38 +++ i386/include/asm.h 19 Dec 2010 20:01:14 - @@ -205,16 +205,14 @@ #ifdef __STDC__ #define WARN_REFERENCES(sym,msg) \ - .stabs msg ## ,30,0,0,0 ; \ - .stabs __STRING(_C_LABEL(sym)) ## ,1,0,0,0 -#elif defined(__ELF__) -#define WARN_REFERENCES(sym,msg) \ - .stabs msg,30,0,0,0 ; \ - .stabs __STRING(sym),1,0,0,0 + .pushsection .gnu.warning. ## sym;\ + .ascii msg; \ + .popsection #else #define WARN_REFERENCES(sym,msg) \ - .stabs msg,30,0,0,0 ; \ - .stabs __STRING(_/**/sym),1,0,0,0 + .pushsection .gnu.warning./**/sym;\ + .ascii msg; \ + .popsection #endif /* __STDC__ */ #endif /* !_I386_ASM_H_ */ Index: m68k/include/asm.h === RCS file: /home/joerg/repo/netbsd/src/sys/arch/m68k/include/asm.h,v retrieving revision 1.25 diff -u -p -r1.25 asm.h --- m68k/include/asm.h 28 Apr 2008 20:23:26 - 1.25 +++ m68k/include/asm.h 19 Dec 2010 20:16:06 - @@ -206,15 +206,15 @@ alias = sym #ifdef __STDC__ -#define __STRING(x) #x #define WARN_REFERE
