Date: Thu, 11 Mar 1999 02:30:02 -0800 (PST) From: Doug Rabson <dfr@nlsystems.com> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/10535: Very poor ethernet performance with tx driver Message-ID: <199903111030.CAA49547@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/10535; it has been noted by GNATS.
From: Doug Rabson <dfr@nlsystems.com>
To: Peter Jeremy <peter.jeremy@auss2.alcatel.com.au>
Cc: FreeBSD-gnats-submit@FreeBSD.ORG, andreas@FreeBSD.ORG
Subject: Re: kern/10535: Very poor ethernet performance with tx driver
Date: Thu, 11 Mar 1999 10:23:10 +0000 (GMT)
On Thu, 11 Mar 1999, Peter Jeremy wrote:
> Doug Rabson <dfr@nlsystems.com> wrote:
> >The fix looks correct to me but as DG noted when the last change was made
> >to this driver, it should be using DELAY to avoid future problems with
> >processor speeds.
>
> I think that's a separate issue. The current code is incorrect because
> references to H/W registers aren't marked volatile - and are therefore
> likely to be optimised away. Andreas' change made the problem show
> up because of the extra delay. I'm surprised that the only symptom
> was poor performance.
The register accesses should certainly be through volatile pointers. Any
number of problems could happen without that, especially if we change
compilers.
>
> >If these are changed to call DELAY(1) in the loop, I think the original
> >patch to increase the loop in epic_init_phy wouldn't be needed either.
>
> I agree that delays should be done using DELAY(). The problem is that
> DELAY() isn't really ideal for the situation in
> epic_{read,write}_phy_register: The code is polling a `ready' bit and
> the limit just makes sure that things don't lock up. (I'm sure
> there's similar code in lots of device drivers). It would be nice if
> the kernel supported something equivalent to setitimer(2)/SIGALRM for
> this purpose.
>
> Based on a quick look at DELAY() (and without knowing the range of
> expected delay's), I suspect that changing the code to do DELAY(1) on
> each loop would worsen the performance - particularly on slow
> processors where the overheads in DELAY() are significant (>1us).
> (Once the bit becomes ready in the PHY, it will typically be (1usec +
> DELAY() overhead)/2 before the state change is seen - compared with
> ~10 clocks for the current code).
You might be right. On the other hand, other code in the same driver uses
DELAY(1) when reading registers, for instance near the beginning of
epic_stop_activity:
for(i=0;i<0x1000;i++) {
if( (CSR_READ_4(sc,INTSTAT) & INTSTAT_RXIDLE) == INTSTAT_RXIDLE )
break;
DELAY(1);
}
Changing the phy accesses to do the same would regularise the code and I'm
convinced that it would be safer as processors get faster.
--
Doug Rabson Mail: dfr@nlsystems.com
Nonlinear Systems Ltd. Phone: +44 181 442 9037
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199903111030.CAA49547>