Re: Introducing localcount(9)

[Chuck Silvers <chuq%chuq.com@localhost>]

On Wed, May 17, 2017 at 07:07:18AM +0800, Paul Goyette wrote:
> On Tue, 16 May 2017, Chuck Silvers wrote:
> 
> > hi paul,
> > 
> > is the intent that in the long term everything should be converted to use
> > the new interfaces and that the non-"acquire" interfaces be removed?
> > (that would be my preference, as well as keeping the original names...
> > the new names are long enough that they make it harder to read the code,
> > at least for me.)
> 
> The intent is that, over time, everything would be changed.
> 
> > if so, is the reason that you aren't converting everything now just that
> > you wanted to keep this change to a managable size?  I could help with
> > converting the rest of the drivers now if that would be useful.
> 
> There are a _lot_ of places to touch!  I've gotten to quite a few, but still
> have literally hundreds of files remaining.  Most of them are fairly
> mechanical, but calls to device_lookup_private() in particular will require
> close attention to ensure that the reference can be released.  In some
> cases, it may require rearranging of some logic. (The changes to cgd(4) come
> to mind here...)
> 
> Rather than trying to rototill everything prior to a single massive commit,
> I felt it was safer to provide additional routines with new names.  Yeah,
> they're a little bit awkward ...

transitions that drag on for years and years aren't good either.
the "device_t/softc split" one would have gone on forever if I hadn't
just converted everything that was left en masse a few years back.
I'm offering to do the work to avoid another unending transition here.


> > a couple more suggestions:
> > 
> > rather than require every driver to use the "DEVSW_MODULE_INIT" hack
> > when defining a devsw structure, it would be better to have bdevsw_attach()
> > and cdevsw_attach() take care of the localcount details without the
> > driver author needing to be aware of them.  I'd also think it would be
> > better to have the d_localcount field be the actual "struct localcount"
> > rather than a pointer to one, since you already enforce that the d_localcount
> > pointer is non-NULL and not shared between structures.
> 
> Initially, the plan was for {b,c}devsw_attach() to allocate the localcount
> structures dynamically.  However, the device structure is const, so it
> wasn't permitted to update the pointer to the localcount.

the easy solution is to just remove the const.
if you don't want do that then you ought to put the localcount somewhere
outside the const devsw structure, but that would be a bigger change.


> For the same reason I was not able to embed the struct localcount directly
> within the struct device.

but you did do that:

struct device {
...
        struct localcount dv_localcnt;  /* reference counter */
};


> > device_acquire() seems of dubious value... it's only safe to use when one
> > already holds a reference on the device_t, so having it as an interface
> > is just inviting people to try to do fancy (and probably wrong) things
> > with the reference counting.  in your branch, you only use it in ld.c
> > (where it's either unnecessary or you're using it unsafely) and in fss.c
> > (which is just working around the fact that config_attach_pseudo()
> > returns a device_t with no reference, and it would seem better to fix
> > config_attach_pseudo() rather than work around that in the fss driver.)
> > I think it would be better to not have a device_acquire() interface
> > exposed to drivers.
> 
> For fss.c:
> 
> If I just change config_attach_pseudo() to always acquire a reference, then
> it would mean updating _every_ caller (to release the reference at some
> time).

there aren't so many of those, it won't be that bad.
I'm volunteering to help with that work as well.


> I could introduce config_attach_pseudo_acquire() but that is definitely an
> awkward routine name!

yea, I'd like to avoid that.


> For ld.c:
> 
> The intent of the calls to device_acquire() was to prevent the ld from
> disappearing and the softc being free()d while the caller is accessing the
> softc pointer.  For each of the five calls in ld.c, it might be unnecessary,
> assuming that the caller of these routines already has its own reference.
> But the extra references don't hurt, and there's no way to KASSERT() that
> the caller already has a reference.
> 
> Perhaps the following variation would be more appropriate?
> 
> Current code:
> 
> static int
> ld_diskstart(device_t dev, struct buf *bp)
> {
> 	struct ld_softc *sc;
> 	int error;
> 
> 	device_acquire(dev);
> 	sc = device_private(dev);
> 	if (sc->sc_queuecnt >= sc->sc_maxqueuecnt) {
> 		device_release(dev);
> 		return EAGAIN;
> 	}
> 	...
> 	device_release(dev);
> 	return error;
> }
> 
> New variation:
> 
> static int
> ld_diskstart(device_t dev, struct buf *bp)
> {
> 	struct ld_softc *sc;
> 	int error;
> > > > 	int unit;
> > > > 	device_t self;
> 
> > > > 	unit = DISKUNIT(bp->b_dev);
> > > > 	self = device_lookup_acquire(&ld_cd, unit);
> > > > 	KASSERT(self == dev);
> 	sc = device_private(dev);
> 	if (sc->sc_queuecnt >= sc->sc_maxqueuecnt) {
> 		device_release(dev);
> 		return EAGAIN;
> 	}
> 	...
> 	device_release(dev);
> 	return error;
> }

that would at least be safe.  but it should still be unnecessary,
since the queue of pending I/O requests has to be drained before
the device_t is freed.

ld_config_interrupts() should be made safe by having the device framework
not free a device_t when work delayed by config_interrupts() is still pending.
we shouldn't force every driver that uses config_interrupts() to
deal with this separately.

ld_discard() can only be invoked on an open device (in the bdev_open /
cdev_open sense), and the device being open has to be enough to keep
the device_t valid.  I'll be surprised if that's not the case already.

the last two (ld_dumpblocks() and ld_iosize()) are only used for kernel
crash dumps, and a device being configured as a dump device should also
involve keeping a reference on the device_t (probably indirectly via
the device being kept open while it is configured as a dump device).


> Which, by the way, begs the question: if the bp->b_dev member already
> contains the device_t we need, why do we pass both dev and bp as args to
> xx_diskstart() routines?   :)

bp->b_dev is a dev_t rather than a device_t, so I imagine that the purpose
was to avoid needing to look up the device_t from the dev_t every time
if the caller always already had the device_t at hand.

-Chuck