lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 2 Jan 2015 14:56:36 -0800
From:	Dave Taht <dave.taht@...il.com>
To:	Felipe Balbi <balbi@...com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Linux OMAP Mailing List <linux-omap@...r.kernel.org>
Subject: Re: [PATCH 3/4] net: ethernet: cpsw: split out IRQ handler

On Fri, Jan 2, 2015 at 11:03 AM, Felipe Balbi <balbi@...com> wrote:
> Hi,
>
> (please use reply-all to keep mailing lists in Cc, also avoid
> top-posting)

I am trying not to read netdev right now... and failing, obviously.

>
> On Fri, Jan 02, 2015 at 10:58:29AM -0800, Dave Taht wrote:
>> The beaglebone only has a 100mbit phy, so you aren't going to get more
>> than that.
>
> very true :-) Still, with AM437x SK which is definitely GigE, I'm
> getting 201Mbits/sec.
>
>> (so do a lot of IoT devices).
>>
>> So you have the two patches that went by on BQL and on NAPI for the beagle?
>
> no, got any pointers ?

the relevant thread was "am335x: cpsw: phy ignores max-speed setting"

and the initial very small BQL enablement patch was here:

https://patchwork.ozlabs.org/patch/407640/

(it needed a saner treatment of a failure to dma something in
cpsw_tx_packet_submit  - the patch as is has also been part of nelsons
trees for the beaglebone for a while)

But it was rightly pointed out later in the thread that this change

+#define CPSW_POLL_WEIGHT 16

made for the biggest part of the improvement, and someone else on the
thread proposed handling that more dynamically for 100mbit phys with
another patch (that I can't find at the moment)

... but the root cause of the excessive latency in this driver was the
single tx/rx dma queue, which you are addressing  in your patch set.
So if you glop on more of the above, mo better, perhaps you will win
bigger.

I will try to slice out some time to boot up a beagle on net-next next week.

>> On Fri, Jan 2, 2015 at 10:55 AM, Felipe Balbi <balbi@...com> wrote:
>> > Hi,
>> >
>> > On Fri, Jan 02, 2015 at 10:49:49AM -0800, Dave Taht wrote:
>> >> +1.
>> >>
>> >> We'd had a thread on netdev (can't find it now) where we discussed
>> >> adding BQL support and also something saner for the NAPI handling to
>> >> this driver.
>> >
>> > yeah, currently is completely borked. I'm on a gigabit network and I'm
>> > getting 94Mbits/sec, total crap.
>> >
>> >> Initial results for the beaglebone black were pretty spectacular, and
>> >> it does look like this is way cleaner infrastructure underneat th deal
>> >> with. Are you testing
>> >
>> > cool, if I new more about networking I'd certainly help, but I can help
>> > testing for sure, just keep me in Cc ;-)
>> >
>> >> on the beaglebone black.? do you remember that convo?
>> >
>> > yeah, testing on beagleboneblack and AM437x SK.
>> >
>> > cheers
>> >
>> >> On Fri, Jan 2, 2015 at 10:10 AM, Felipe Balbi <balbi@...com> wrote:
>> >> > Now we can introduce dedicated IRQ handlers
>> >> > for each of the IRQ events. This helps with
>> >> > cleaning up a little bit of the clutter in
>> >> > cpsw_interrupt() while also making sure that
>> >> > TX IRQs will try to handle TX buffers while
>> >> > RX IRQs will try to handle RX buffers.
>> >> >
>> >> > Signed-off-by: Felipe Balbi <balbi@...com>
>> >> > ---
>> >> >  drivers/net/ethernet/ti/cpsw.c | 41 ++++++++++++++++++++++++++++++-----------
>> >> >  1 file changed, 30 insertions(+), 11 deletions(-)
>> >> >
>> >> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> >> > index 6e04128..c9081bd 100644
>> >> > --- a/drivers/net/ethernet/ti/cpsw.c
>> >> > +++ b/drivers/net/ethernet/ti/cpsw.c
>> >> > @@ -754,18 +754,36 @@ requeue:
>> >> >                 dev_kfree_skb_any(new_skb);
>> >> >  }
>> >> >
>> >> > -static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
>> >> > +static irqreturn_t cpsw_dummy_interrupt(int irq, void *dev_id)
>> >> >  {
>> >> >         struct cpsw_priv *priv = dev_id;
>> >> >         int value = irq - priv->irqs_table[0];
>> >> >
>> >> > -       /* NOTICE: Ending IRQ here. The trick with the 'value' variable above
>> >> > -        * is to make sure we will always write the correct value to the EOI
>> >> > -        * register. Namely 0 for RX_THRESH Interrupt, 1 for RX Interrupt, 2
>> >> > -        * for TX Interrupt and 3 for MISC Interrupt.
>> >> > -        */
>> >> >         cpdma_ctlr_eoi(priv->dma, value);
>> >> >
>> >> > +       return IRQ_HANDLED;
>> >> > +}
>> >> > +
>> >> > +static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id)
>> >> > +{
>> >> > +       struct cpsw_priv *priv = dev_id;
>> >> > +
>> >> > +       cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX);
>> >> > +       cpdma_chan_process(priv->txch, 128);
>> >> > +
>> >> > +       priv = cpsw_get_slave_priv(priv, 1);
>> >> > +       if (priv)
>> >> > +               cpdma_chan_process(priv->txch, 128);
>> >> > +
>> >> > +       return IRQ_HANDLED;
>> >> > +}
>> >> > +
>> >> > +static irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id)
>> >> > +{
>> >> > +       struct cpsw_priv *priv = dev_id;
>> >> > +
>> >> > +       cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_RX);
>> >> > +
>> >> >         cpsw_intr_disable(priv);
>> >> >         if (priv->irq_enabled == true) {
>> >> >                 cpsw_disable_irq(priv);
>> >> > @@ -1617,7 +1635,8 @@ static void cpsw_ndo_poll_controller(struct net_device *ndev)
>> >> >
>> >> >         cpsw_intr_disable(priv);
>> >> >         cpdma_ctlr_int_ctrl(priv->dma, false);
>> >> > -       cpsw_interrupt(ndev->irq, priv);
>> >> > +       cpsw_rx_interrupt(priv->irq[1], priv);
>> >> > +       cpsw_tx_interrupt(priv->irq[2], priv);
>> >> >         cpdma_ctlr_int_ctrl(priv->dma, true);
>> >> >         cpsw_intr_enable(priv);
>> >> >  }
>> >> > @@ -2351,7 +2370,7 @@ static int cpsw_probe(struct platform_device *pdev)
>> >> >                 goto clean_ale_ret;
>> >> >
>> >> >         priv->irqs_table[0] = irq;
>> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
>> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
>> >> >                         0, dev_name(&pdev->dev), priv);
>> >> >         if (ret < 0) {
>> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
>> >> > @@ -2363,7 +2382,7 @@ static int cpsw_probe(struct platform_device *pdev)
>> >> >                 goto clean_ale_ret;
>> >> >
>> >> >         priv->irqs_table[1] = irq;
>> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
>> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_rx_interrupt,
>> >> >                         0, dev_name(&pdev->dev), priv);
>> >> >         if (ret < 0) {
>> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
>> >> > @@ -2375,7 +2394,7 @@ static int cpsw_probe(struct platform_device *pdev)
>> >> >                 goto clean_ale_ret;
>> >> >
>> >> >         priv->irqs_table[2] = irq;
>> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
>> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_tx_interrupt,
>> >> >                         0, dev_name(&pdev->dev), priv);
>> >> >         if (ret < 0) {
>> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
>> >> > @@ -2387,7 +2406,7 @@ static int cpsw_probe(struct platform_device *pdev)
>> >> >                 goto clean_ale_ret;
>> >> >
>> >> >         priv->irqs_table[3] = irq;
>> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
>> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
>> >> >                         0, dev_name(&pdev->dev), priv);
>> >> >         if (ret < 0) {
>> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
>> >> > --
>> >> > 2.2.0
>> >> >
>> >> > --
>> >> > To unsubscribe from this list: send the line "unsubscribe netdev" in
>> >> > the body of a message to majordomo@...r.kernel.org
>> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>
>> >>
>> >>
>> >> --
>> >> Dave Täht
>> >>
>> >> thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
>> >
>> > --
>> > balbi
>>
>>
>>
>> --
>> Dave Täht
>>
>> thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
>
> --
> balbi



-- 
Dave Täht

thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists