Date: Mon, 25 Aug 2014 14:10:01 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com>, freebsd-hackers@freebsd.org Subject: Re: atomic_load_acq_int in sequential_heuristic Message-ID: <20140825111000.GC2737@kib.kiev.ua> In-Reply-To: <20140825091056.GC14344@dft-labs.eu> References: <20140824115729.GC2045@dft-labs.eu> <20140824162331.GW2737@kib.kiev.ua> <20140824164236.GX2737@kib.kiev.ua> <20140825005659.GA14344@dft-labs.eu> <20140825073404.GZ2737@kib.kiev.ua> <20140825081526.GB14344@dft-labs.eu> <20140825083539.GB2737@kib.kiev.ua> <20140825091056.GC14344@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--tTm07/aDcFLy+pwu
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Mon, Aug 25, 2014 at 11:10:56AM +0200, Mateusz Guzik wrote:
> On Mon, Aug 25, 2014 at 11:35:39AM +0300, Konstantin Belousov wrote:
> > > + atomic_set_int(&fp->f_flag, FHASLOCK);
> > You misspelled FRDAHEAD as FHASLOCK, below as well.
> > Was this tested ?
> >=20
>=20
> Oops, damn copy-pasto. Sorry.
>=20
> > > + VOP_UNLOCK(vp, 0);
> > > } else {
> > > - do {
> > > - new =3D old =3D fp->f_flag;
> > > - new &=3D ~FRDAHEAD;
> > > - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new));
> > > + atomic_clear_int(&fp->f_flag, FHASLOCK);
> > So what about extending the vnode lock to cover the flag reset ?
> >=20
>=20
> Sure.
>=20
> So this time I tested it properly and found out it is impossible to
> disable the hint. The test is:
>=20
> -1 is truncated and then read from intptr_t which yields a big positive
> number instead. Other users in the function use int tmp to work around
> this issue.
Could you provide me with the test case which demonstrates the problem ?
The fcntl(2) prototype in sys/fcntl.h is variadic, so int arg argument
is not promoted. On the other hand, syscalls.master declares arg as long.
Did you tried to pass -1L as third argument to disable ?
>=20
> diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
> index 7abdca0..774f647 100644
> --- a/sys/kern/kern_descrip.c
> +++ b/sys/kern/kern_descrip.c
> @@ -760,7 +760,8 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr=
_t arg)
> error =3D EBADF;
> break;
> }
> - if (arg >=3D 0) {
> + tmp =3D arg;
> + if (tmp >=3D 0) {
> vp =3D fp->f_vnode;
> error =3D vn_lock(vp, LK_SHARED);
> if (error !=3D 0) {
> @@ -769,7 +770,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr=
_t arg)
> }
> bsize =3D fp->f_vnode->v_mount->mnt_stat.f_iosize;
> VOP_UNLOCK(vp, 0);
> - fp->f_seqcount =3D (arg + bsize - 1) / bsize;
> + fp->f_seqcount =3D (tmp + bsize - 1) / bsize;
> do {
> new =3D old =3D fp->f_flag;
> new |=3D FRDAHEAD;
>=20
> Then the patch in question:
>=20
> diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
> index 774f647..4efadb0 100644
> --- a/sys/kern/kern_descrip.c
> +++ b/sys/kern/kern_descrip.c
> @@ -476,7 +476,6 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr=
_t arg)
> struct vnode *vp;
> cap_rights_t rights;
> int error, flg, tmp;
> - u_int old, new;
> uint64_t bsize;
> off_t foffset;
> =20
> @@ -760,27 +759,25 @@ kern_fcntl(struct thread *td, int fd, int cmd, intp=
tr_t arg)
> error =3D EBADF;
> break;
> }
> + vp =3D fp->f_vnode;
> + /*
> + * Exclusive lock synchronizes against
> + * sequential_heuristic().
I would also add a sentence that we care about f_seqcount update in
seq_heur().
Another place to add the locking annotation is the struct file in
sys/file.h. Now f_seqcount is 'protected' by the vnode lock.
I am not sure how to express the locking model shortly.
> + */
> + error =3D vn_lock(vp, LK_EXCLUSIVE);
> + if (error !=3D 0) {
> + fdrop(fp, td);
> + break;
> + }
> tmp =3D arg;
> if (tmp >=3D 0) {
> - vp =3D fp->f_vnode;
> - error =3D vn_lock(vp, LK_SHARED);
> - if (error !=3D 0) {
> - fdrop(fp, td);
> - break;
> - }
> bsize =3D fp->f_vnode->v_mount->mnt_stat.f_iosize;
> - VOP_UNLOCK(vp, 0);
> fp->f_seqcount =3D (tmp + bsize - 1) / bsize;
> - do {
> - new =3D old =3D fp->f_flag;
> - new |=3D FRDAHEAD;
> - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new));
> + atomic_set_int(&fp->f_flag, FRDAHEAD);
> } else {
> - do {
> - new =3D old =3D fp->f_flag;
> - new &=3D ~FRDAHEAD;
> - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new));
> + atomic_clear_int(&fp->f_flag, FRDAHEAD);
> }
> + VOP_UNLOCK(vp, 0);
> fdrop(fp, td);
> break;
> =20
> diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c
> index f1d19ac..98823f3 100644
> --- a/sys/kern/vfs_vnops.c
> +++ b/sys/kern/vfs_vnops.c
> @@ -438,7 +438,8 @@ static int
> sequential_heuristic(struct uio *uio, struct file *fp)
> {
> =20
> - if (atomic_load_acq_int(&(fp->f_flag)) & FRDAHEAD)
> + ASSERT_VOP_LOCKED(fp->f_vnode, __func__);
> + if (fp->f_flag & FRDAHEAD)
> return (fp->f_seqcount << IO_SEQSHIFT);
> =20
> /*
> --=20
> Mateusz Guzik <mjguzik gmail.com>
--tTm07/aDcFLy+pwu
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBAgAGBQJT+xmIAAoJEJDCuSvBvK1BmZwP/2LKzDdUErkIZr4iCkFRDLUr
xmEngq42kKdrcDPSKjI6j9n7n8k5kSXMl93h2G3t9Q/X526bMmXzaP3gUnLBxJOc
ahA2UxBlIT8FC9GxdOFy/m1bBq+i+X1+dnrDtImyYYN2Wh0b6Xw/QvQtiji58Qh1
gGDX/cFENJcXcgEBnZgMNyVeHxjnBfRF1F6cEGVExxLbg0b5M5VmF8oxTQnlxyKe
1GV1PYC2CiFCDkx/lFB85hRjYCTxXyak0sJIH6ySkXTU1vxlcKfWTtWqz6xRG6H8
HHrkLWLID5gdhSK97u7z0IcYSY1ZvSMu3/d7FpeHnS/hgsxg0GtPdkiZhNwTlgoJ
fTsiZuvkUBTTKYnhLzGvxkbwaVaOS0zAKsiONynscKXmKIyELRiUSL/mxi83stPw
/P7rvGb72rCt5W2ezWh9OjtKvNJVlaHTRh7YWk3213Bgs9VqkSN4dYceHpdkB+XN
jOD5T54gB8vLeZYVr4eopju1RLT77O/kKBCLnuuf+hlPv6kt62VZOJrE0P6WOc6m
8jluxqU0//wRx83gADmnHX4P4hix4C20qw23gDSGubCaBy4qzCDiewyyIbyAlM79
QTKIJnh338HNTjQz1mSbpcPEAMof2ZFRjYDTknodj1LyrQp83QQHDKnSLF8EZJUR
F3rRTJnkzUJIidBU7MeL
=i1pV
-----END PGP SIGNATURE-----
--tTm07/aDcFLy+pwu--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140825111000.GC2737>