atomic_load_acq_int in sequential_heuristic

[Mateusz Guzik]

On Mon, Aug 25, 2014 at 02:10:01PM +0300, Konstantin Belousov wrote:
> 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 ?
> > > 
> > 
> > Oops, damn copy-pasto. Sorry.
> > 
> > > > +			VOP_UNLOCK(vp, 0);
> > > >  		} else {
> > > > -			do {
> > > > -				new = old = fp->f_flag;
> > > > -				new &= ~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 ?
> > > 
> > 
> > Sure.
> > 
> > So this time I tested it properly and found out it is impossible to
> > disable the hint. The test is:
> > 
> > -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 ?
> 

Nothing special:
https://people.freebsd.org/~mjg/patches/F_READAHEAD.c

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

Yes, -1L deals with the problem. I would still argue that using 'tmp'
like the rest of the function would not hurt as a cheap solution.

> > 
> > 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 = EBADF;
> >  			break;
> >  		}
> > -		if (arg >= 0) {
> > +		tmp = arg;
> > +		if (tmp >= 0) {
> >  			vp = fp->f_vnode;
> >  			error = vn_lock(vp, LK_SHARED);
> >  			if (error != 0) {
> > @@ -769,7 +770,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
> >  			}
> >  			bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize;
> >  			VOP_UNLOCK(vp, 0);
> > -			fp->f_seqcount = (arg + bsize - 1) / bsize;
> > +			fp->f_seqcount = (tmp + bsize - 1) / bsize;
> >  			do {
> >  				new = old = fp->f_flag;
> >  				new |= FRDAHEAD;
> > 
> > Then the patch in question:
> > 
> > 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;
> >  
> > @@ -760,27 +759,25 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
> >  			error = EBADF;
> >  			break;
> >  		}
> > +		vp = 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().
> 

/*
 * Exclusive lock synchronizes against f_seqcount reads and writes in
 * sequential_heuristic().
 */

> 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.
> 

/*
 * (a) f_vnode lock required (shared allows both reads and writes)
 */


-- 
Mateusz Guzik <mjguzik gmail.com>