Re: an(4): tsleep(9) -> tsleep_nsec(9)
On Thu, Nov 26, 2020 at 08:25:48PM +1100, Jonathan Gray wrote:
> On Tue, Nov 24, 2020 at 07:20:46PM -0600, Scott Cheloha wrote:
> > Hi,
> >
> > Both kettenis@ and mpi@ have mentioned in private that my proposed
> > changes to tsleep_nsec(9) etc. would be nicer if we could just get rid
> > of tsleep(9) etc. entirely.
> >
> > This is difficult, but I'll try.
> >
> > Worst case, we thin out the remaining callers. There are not many
> > left.
> >
> > --
> >
> > So, an(4) is one such caller.
> >
> > In an_wait() we spin for (3 * hz) ticks waiting for CSR_WRITE_2 to
> > return the AN_EV_CMD flag. There is no code handling a case where
> > this fails to happen.
> >
> > What we do in practice is very nearly equivalent to spinning for 3
> > seconds waiting for CSR_WRITE_2 to return the AN_EV_CMD flag, so I
> > have converted it to use tsleep_nsec(9).
> >
> > This compiles on amd64 but I can't test it.
> >
> > Thoughts? ok?
>
> I don't see why the upper bound would have to be so precise.
>
> Why not just
>
> for (i = 0; i < 3000; i += 100) {
> if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> break;
> tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(100));
> }
I was just trying to imitate the current behavior as closely as
possible.
If you're fine fudging it like that then I'm fine with it. Just
beware that that tsleep_nsec(9) can and will oversleep by up to 1/hz
seconds.
Did you intend to increase the poll interval from 10ms to 100ms or is
that a typo?
Re: an(4): tsleep(9) -> tsleep_nsec(9)
> Date: Thu, 26 Nov 2020 10:29:50 +0100
> From: Claudio Jeker
>
> On Thu, Nov 26, 2020 at 08:25:48PM +1100, Jonathan Gray wrote:
> > On Tue, Nov 24, 2020 at 07:20:46PM -0600, Scott Cheloha wrote:
> > > Hi,
> > >
> > > Both kettenis@ and mpi@ have mentioned in private that my proposed
> > > changes to tsleep_nsec(9) etc. would be nicer if we could just get rid
> > > of tsleep(9) etc. entirely.
> > >
> > > This is difficult, but I'll try.
> > >
> > > Worst case, we thin out the remaining callers. There are not many
> > > left.
> > >
> > > --
> > >
> > > So, an(4) is one such caller.
> > >
> > > In an_wait() we spin for (3 * hz) ticks waiting for CSR_WRITE_2 to
> > > return the AN_EV_CMD flag. There is no code handling a case where
> > > this fails to happen.
> > >
> > > What we do in practice is very nearly equivalent to spinning for 3
> > > seconds waiting for CSR_WRITE_2 to return the AN_EV_CMD flag, so I
> > > have converted it to use tsleep_nsec(9).
> > >
> > > This compiles on amd64 but I can't test it.
> > >
> > > Thoughts? ok?
> >
> > I don't see why the upper bound would have to be so precise.
> >
> > Why not just
> >
> > for (i = 0; i < 3000; i += 100) {
> > if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> > break;
> > tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(100));
> > }
> >
>
> Agreed, that seems more in the spirit of such wait loops.
+1
> > > Index: an.c
> > > ===
> > > RCS file: /cvs/src/sys/dev/ic/an.c,v
> > > retrieving revision 1.76
> > > diff -u -p -r1.76 an.c
> > > --- an.c 10 Jul 2020 13:26:37 - 1.76
> > > +++ an.c 25 Nov 2020 01:19:16 -
> > > @@ -678,13 +678,18 @@ an_linkstat_intr(struct an_softc *sc)
> > > void
> > > an_wait(struct an_softc *sc)
> > > {
> > > - int i;
> > > + struct timespec now, end;
> > > +
> > > + nanouptime(&now);
> > > + end = now;
> > > + end.tv_sec += 3;/* spin for at most three seconds */
> > >
> > > CSR_WRITE_2(sc, AN_COMMAND, AN_CMD_NOOP2);
> > > - for (i = 0; i < 3*hz; i++) {
> > > - if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> > > + while ((CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD) == 0) {
> > > + nanouptime(&now);
> > > + if (timespeccmp(&end, &now, <=))
> > > break;
> > > - (void)tsleep(sc, PWAIT, "anatch", 1);
> > > + tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(10));
> > > }
> > > CSR_WRITE_2(sc, AN_EVENT_ACK, AN_EV_CMD);
> > > }
> > >
> > >
> >
>
> --
> :wq Claudio
>
>
Re: an(4): tsleep(9) -> tsleep_nsec(9)
On Thu, Nov 26, 2020 at 08:25:48PM +1100, Jonathan Gray wrote:
> On Tue, Nov 24, 2020 at 07:20:46PM -0600, Scott Cheloha wrote:
> > Hi,
> >
> > Both kettenis@ and mpi@ have mentioned in private that my proposed
> > changes to tsleep_nsec(9) etc. would be nicer if we could just get rid
> > of tsleep(9) etc. entirely.
> >
> > This is difficult, but I'll try.
> >
> > Worst case, we thin out the remaining callers. There are not many
> > left.
> >
> > --
> >
> > So, an(4) is one such caller.
> >
> > In an_wait() we spin for (3 * hz) ticks waiting for CSR_WRITE_2 to
> > return the AN_EV_CMD flag. There is no code handling a case where
> > this fails to happen.
> >
> > What we do in practice is very nearly equivalent to spinning for 3
> > seconds waiting for CSR_WRITE_2 to return the AN_EV_CMD flag, so I
> > have converted it to use tsleep_nsec(9).
> >
> > This compiles on amd64 but I can't test it.
> >
> > Thoughts? ok?
>
> I don't see why the upper bound would have to be so precise.
>
> Why not just
>
> for (i = 0; i < 3000; i += 100) {
> if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> break;
> tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(100));
> }
>
Agreed, that seems more in the spirit of such wait loops.
> >
> > Index: an.c
> > ===
> > RCS file: /cvs/src/sys/dev/ic/an.c,v
> > retrieving revision 1.76
> > diff -u -p -r1.76 an.c
> > --- an.c10 Jul 2020 13:26:37 - 1.76
> > +++ an.c25 Nov 2020 01:19:16 -
> > @@ -678,13 +678,18 @@ an_linkstat_intr(struct an_softc *sc)
> > void
> > an_wait(struct an_softc *sc)
> > {
> > - int i;
> > + struct timespec now, end;
> > +
> > + nanouptime(&now);
> > + end = now;
> > + end.tv_sec += 3;/* spin for at most three seconds */
> >
> > CSR_WRITE_2(sc, AN_COMMAND, AN_CMD_NOOP2);
> > - for (i = 0; i < 3*hz; i++) {
> > - if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> > + while ((CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD) == 0) {
> > + nanouptime(&now);
> > + if (timespeccmp(&end, &now, <=))
> > break;
> > - (void)tsleep(sc, PWAIT, "anatch", 1);
> > + tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(10));
> > }
> > CSR_WRITE_2(sc, AN_EVENT_ACK, AN_EV_CMD);
> > }
> >
> >
>
--
:wq Claudio
Re: an(4): tsleep(9) -> tsleep_nsec(9)
On Tue, Nov 24, 2020 at 07:20:46PM -0600, Scott Cheloha wrote:
> Hi,
>
> Both kettenis@ and mpi@ have mentioned in private that my proposed
> changes to tsleep_nsec(9) etc. would be nicer if we could just get rid
> of tsleep(9) etc. entirely.
>
> This is difficult, but I'll try.
>
> Worst case, we thin out the remaining callers. There are not many
> left.
>
> --
>
> So, an(4) is one such caller.
>
> In an_wait() we spin for (3 * hz) ticks waiting for CSR_WRITE_2 to
> return the AN_EV_CMD flag. There is no code handling a case where
> this fails to happen.
>
> What we do in practice is very nearly equivalent to spinning for 3
> seconds waiting for CSR_WRITE_2 to return the AN_EV_CMD flag, so I
> have converted it to use tsleep_nsec(9).
>
> This compiles on amd64 but I can't test it.
>
> Thoughts? ok?
I don't see why the upper bound would have to be so precise.
Why not just
for (i = 0; i < 3000; i += 100) {
if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
break;
tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(100));
}
>
> Index: an.c
> ===
> RCS file: /cvs/src/sys/dev/ic/an.c,v
> retrieving revision 1.76
> diff -u -p -r1.76 an.c
> --- an.c 10 Jul 2020 13:26:37 - 1.76
> +++ an.c 25 Nov 2020 01:19:16 -
> @@ -678,13 +678,18 @@ an_linkstat_intr(struct an_softc *sc)
> void
> an_wait(struct an_softc *sc)
> {
> - int i;
> + struct timespec now, end;
> +
> + nanouptime(&now);
> + end = now;
> + end.tv_sec += 3;/* spin for at most three seconds */
>
> CSR_WRITE_2(sc, AN_COMMAND, AN_CMD_NOOP2);
> - for (i = 0; i < 3*hz; i++) {
> - if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> + while ((CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD) == 0) {
> + nanouptime(&now);
> + if (timespeccmp(&end, &now, <=))
> break;
> - (void)tsleep(sc, PWAIT, "anatch", 1);
> + tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(10));
> }
> CSR_WRITE_2(sc, AN_EVENT_ACK, AN_EV_CMD);
> }
>
>
an(4): tsleep(9) -> tsleep_nsec(9)
Hi,
Both kettenis@ and mpi@ have mentioned in private that my proposed
changes to tsleep_nsec(9) etc. would be nicer if we could just get rid
of tsleep(9) etc. entirely.
This is difficult, but I'll try.
Worst case, we thin out the remaining callers. There are not many
left.
--
So, an(4) is one such caller.
In an_wait() we spin for (3 * hz) ticks waiting for CSR_WRITE_2 to
return the AN_EV_CMD flag. There is no code handling a case where
this fails to happen.
What we do in practice is very nearly equivalent to spinning for 3
seconds waiting for CSR_WRITE_2 to return the AN_EV_CMD flag, so I
have converted it to use tsleep_nsec(9).
This compiles on amd64 but I can't test it.
Thoughts? ok?
Index: an.c
===
RCS file: /cvs/src/sys/dev/ic/an.c,v
retrieving revision 1.76
diff -u -p -r1.76 an.c
--- an.c10 Jul 2020 13:26:37 - 1.76
+++ an.c25 Nov 2020 01:19:16 -
@@ -678,13 +678,18 @@ an_linkstat_intr(struct an_softc *sc)
void
an_wait(struct an_softc *sc)
{
- int i;
+ struct timespec now, end;
+
+ nanouptime(&now);
+ end = now;
+ end.tv_sec += 3;/* spin for at most three seconds */
CSR_WRITE_2(sc, AN_COMMAND, AN_CMD_NOOP2);
- for (i = 0; i < 3*hz; i++) {
- if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
+ while ((CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD) == 0) {
+ nanouptime(&now);
+ if (timespeccmp(&end, &now, <=))
break;
- (void)tsleep(sc, PWAIT, "anatch", 1);
+ tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(10));
}
CSR_WRITE_2(sc, AN_EVENT_ACK, AN_EV_CMD);
}
an(4): tsleep(9) -> tsleep_nsec(9)
In practice, how many ticks will we poll for this device to be ready?
Does it vary by parent device, e.g. pci vs isapnp vs pcmcia?
If it's usually 1 we could try to poll at a faster rate, say 1ms or
5ms. If it's usually 2 or more we can leave it at 10ms, the effective
rate on 100hz platforms.
Thoughts? ok?
Index: ic/an.c
===
RCS file: /cvs/src/sys/dev/ic/an.c,v
retrieving revision 1.75
diff -u -p -r1.75 an.c
--- ic/an.c 7 Nov 2019 12:56:34 - 1.75
+++ ic/an.c 16 Jan 2020 16:47:08 -
@@ -681,10 +681,10 @@ an_wait(struct an_softc *sc)
int i;
CSR_WRITE_2(sc, AN_COMMAND, AN_CMD_NOOP2);
- for (i = 0; i < 3*hz; i++) {
+ for (i = 0; i < 300; i++) {
if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
break;
- (void)tsleep(sc, PWAIT, "anatch", 1);
+ tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(10));
}
CSR_WRITE_2(sc, AN_EVENT_ACK, AN_EV_CMD);
}
