[PATCH 2/2] netdev/fec: fix performance impact from mdio poll operation
Bryan Wu
bryan.wu at canonical.com
Tue Apr 13 23:25:33 UTC 2010
On 04/13/2010 03:57 PM, Eric Miao wrote:
> On Wed, Apr 14, 2010 at 5:37 AM, Bryan Wu<bryan.wu at canonical.com> wrote:
>> On 04/13/2010 12:09 PM, Andy Whitcroft wrote:
>>> On Thu, Apr 08, 2010 at 02:44:59PM +0800, Bryan Wu wrote:
>>>> BugLink: http://bugs.launchpad.net/bugs/546649
>>>> BugLink: http://bugs.launchpad.net/bugs/457878
>>>>
>>>> After introducing phylib supporting, users experienced performace drop. That is
>>>> because of the mdio polling operation of phylib. Use msleep to replace the busy
>>>> waiting cpu_relax() and remove the warning message.
>>>>
>>>> Signed-off-by: Bryan Wu<bryan.wu at canonical.com>
>>>> ---
>>>> drivers/net/fec.c | 45 +++++++++++++++++++++------------------------
>>>> 1 files changed, 21 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
>>>> index 53240d3..2280373 100644
>>>> --- a/drivers/net/fec.c
>>>> +++ b/drivers/net/fec.c
>>>> @@ -216,7 +216,7 @@ static void fec_stop(struct net_device *dev);
>>>> #define FEC_MMFR_TA (2<< 16)
>>>> #define FEC_MMFR_DATA(v) (v& 0xffff)
>>>>
>>>> -#define FEC_MII_TIMEOUT 10000
>>>> +#define FEC_MII_TIMEOUT 10
>>>>
>>>> /* Transmitter timeout */
>>>> #define TX_TIMEOUT (2 * HZ)
>>>> @@ -624,13 +624,29 @@ spin_unlock:
>>>> /*
>>>> * NOTE: a MII transaction is during around 25 us, so polling it...
>>>> */
>>>> -static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>>>> +static int fec_enet_mdio_poll(struct fec_enet_private *fep)
>>>> {
>>>> - struct fec_enet_private *fep = bus->priv;
>>>> int timeout = FEC_MII_TIMEOUT;
>>>>
>>>> fep->mii_timeout = 0;
>>>>
>>>> + /* wait for end of transfer */
>>>> + while (!(readl(fep->hwp + FEC_IEVENT)& FEC_ENET_MII)) {
>>>> + msleep(1);
>>>> + if (timeout--< 0) {
>>>> + fep->mii_timeout = 1;
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>
> This is ugly, I'd suggest wait_event_timeout(), though the response
> time will be worse instead.
>
Oh, I don't think it is ugly, since you can find numbers of such code in polling
functions (not only mdio, but also SPI, I2C etc). Sometime we have to wait for a
flag. If we busy wait such as cpu_relax() which is barrier() on ARM, it will
cause the system response and transfer performance drop in our Babbage board.
The drop is not because the busy polling is because the hardware is not perfect,
especially when we transfer large files. The problem might be the timeout trying
number 100000 is too large, busy loop in the read_mdio function is too long when
the hardware is not perfect.
So if the mdio polling timeout, the phylib timer will try read_mdio shortly.
That's a polling method to monitor the ethernet phy link status change.
Generally, this kind of timeout is an acceptable situation, we will recover it
when we restart read_mdio again.
> msleep(1) is almost equivalent on a system with HZ == 100.
>
Actually, I don't wanna a very high resolution time delay, I just wanna sleep a
very short time and check again the flag. There is no usleep(), msleep(1) is
friendly here.
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>>>> +{
>>>> + struct fec_enet_private *fep = bus->priv;
>>>> +
>>>> +
>>>> /* clear MII end of transfer bit*/
>>>> writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
>>>>
>>>> @@ -639,15 +655,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>>>> FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
>>>> FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
>>>>
>>>> - /* wait for end of transfer */
>>>> - while (!(readl(fep->hwp + FEC_IEVENT)& FEC_ENET_MII)) {
>>>> - cpu_relax();
>>>> - if (timeout--< 0) {
>>>> - fep->mii_timeout = 1;
>>>> - printk(KERN_ERR "FEC: MDIO read timeout\n");
>>>> - return -ETIMEDOUT;
>>>> - }
>>>> - }
>
> I think polling will instead improve the performance (by waiting less once
> the event happens), why is it causing performance drop instead?
>
It should be, but if we wait for a flag too long especially the hardware is not
perfect, it will cause the performance drop. That is we observed in babbage board.
>>>> + fec_enet_mdio_poll(fep);
>>>
>>> You have pulled out this timeout loop, but in the replacement there is
>>> not indication that it has timed-out. In the old code we would have
>>> returned -ETIMEDOUT and not touched MII_DATA (below). In the new we
>>> will timeout, mark mii_timeout but not return an indication of that, not
>>> would we note it here, and so we would drop through and touch MII_DATA.
>>> Is this an intentional change of semantics?
>>>
>>
>> Since the timeout thing is not an error just a warning, the caller in phylib
>> timer will call this function shortly and normally ignore the negative return
>> value. I just looked up this busy timeout things in drivers/net/.
>>
>
> returning -1 is always a bad idea, -1 means -EPERM, which is strongly
> undesirable. -ETIMEDOUT, I'd say, is a lot better in any sense.
>
-ETIMEDOUT is the best if we return some thing when it is timeout.
-Bryan
>> drivers/net/davinci_emac.c:
>> it uses endless loop:
>> /* Wait until mdio is ready for next command */
>> #define MDIO_WAIT_FOR_USER_ACCESS\
>> while ((emac_mdio_read((MDIO_USERACCESS(0)))&\
>> MDIO_USERACCESS_GO) != 0)
>>
>>
>> drivers/net/au1000_eth.c
>> it returns -1 when timeout
>>
>> drivers/net/arm/at91_ether.c
>> it is the same as us, don't return any thing if timeout.
>>
>> So actually, I am not sure about the return value requirement for this function.
>> And I found several timeout warning when we do large file transfer. The hardware
>> is not perfect.
>>
>> But I will try to check the return value of the poll function, and let you guys
>> know the result.
>>
>> Thanks,
>> -Bryan
>>
More information about the kernel-team
mailing list