[PATCH 2/2] netdev/fec: fix performance impact from mdio poll operation

[Eric Miao]

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.

msleep(1) is almost equivalent on a system with HZ == 100.

>>> +    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?

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

> 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
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>