Re: Introducing localcount(9)

[Paul Goyette <paul%whooppee.com@localhost>]

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


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

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

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

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

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;
}


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


+------------------+--------------------------+----------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:          |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+------------------+--------------------------+----------------------------+