Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1266853 > unrolled thread

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller Francois Romieu <romieu@fr.zoreil.com> - 2015-11-11 00:40 +0100
    Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 01:50 +0100
      Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller David Miller <davem@davemloft.net> - 2015-11-11 03:20 +0100
        Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 13:30 +0100
          Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 14:10 +0100
            Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-11 14:40 +0100
              Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 14:50 +0100
                Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-11 15:10 +0100
                  Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 15:20 +0100
                    Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 15:40 +0100
                      Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-11 15:50 +0100
                    Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-11 15:50 +0100
            Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller David Miller <davem@davemloft.net> - 2015-11-11 17:30 +0100
              Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 19:30 +0100
                Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 20:10 +0100
                  Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller David Miller <davem@davemloft.net> - 2015-11-11 20:20 +0100
                    Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 20:20 +0100
                      Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller David Miller <davem@davemloft.net> - 2015-11-11 20:30 +0100
                        Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller David Miller <davem@davemloft.net> - 2015-11-11 20:30 +0100
                          Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 20:40 +0100
                            Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller David Miller <davem@davemloft.net> - 2015-11-11 20:50 +0100
                              Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 21:50 +0100
                        Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 20:30 +0100
                Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller David Miller <davem@davemloft.net> - 2015-11-11 20:10 +0100
          Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller David Miller <davem@davemloft.net> - 2015-11-11 17:30 +0100

Page 1 of 2  [1] 2  Next page →

#1266853 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

Mans Rullgard <mans@mansr.com> :
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> new file mode 100644
> index 0000000..11cd389
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/nb8800.c
[...]
> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
[...]
> +
> +	netdev_sent_queue(dev, skb->len);
> +
> +	smp_mb__before_spinlock();
> +	spin_lock_irqsave(&priv->tx_lock, flags);

At some point you may consider performing both Tx and Rx from napi context
and thus replacing priv->tx_lock with netif_tx_lock.

> +
> +	if (!skb->xmit_more) {
> +		priv->tx_chain->ready = true;
> +		priv->tx_chain = NULL;
> +		nb8800_tx_dma_start(dev);
> +	}
> +
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
> +	priv->tx_next = next;

Are there strong reasons why nb8800_tx_done could not kick between
spin_unlock_irqrestore and the non-local update of priv->tx_next ?

[...]
> +static irqreturn_t nb8800_irq(int irq, void *dev_id)
> +{
> +	struct net_device *dev = dev_id;
> +	struct nb8800_priv *priv = netdev_priv(dev);
> +	u32 val;
> +
> +	/* tx interrupt */
> +	val = nb8800_readl(priv, NB8800_TXC_SR);
> +	if (val) {
[...]
> +	}
> +
> +	/* rx interrupt */
> +	val = nb8800_readl(priv, NB8800_RXC_SR);
> +	if (val) {
[...]
> +	}
> +
> +	return IRQ_HANDLED;

Returning IRQ_HANDLED is fine if one of those hold:
1. you're sure that at least one of the "if" branch is used
2. you'll be able to quickly figure out what's happening whenever 1. stops
   being true.

-- 
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1266904 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

Francois Romieu <romieu@fr.zoreil.com> writes:

> Mans Rullgard <mans@mansr.com> :
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>> new file mode 100644
>> index 0000000..11cd389
>> --- /dev/null
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
> [...]
>> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
> [...]
>> +
>> +	netdev_sent_queue(dev, skb->len);
>> +
>> +	smp_mb__before_spinlock();
>> +	spin_lock_irqsave(&priv->tx_lock, flags);
>
> At some point you may consider performing both Tx and Rx from napi context
> and thus replacing priv->tx_lock with netif_tx_lock.

That lock is to synchronise the DMA start between nb8800_xmit() and the
interrupt handler.  When the DMA complete interrupt arrives, the next
chain should be kicked off as quickly as possible, and I don't see why
that would benefit from being done in napi context.

>> +
>> +	if (!skb->xmit_more) {
>> +		priv->tx_chain->ready = true;
>> +		priv->tx_chain = NULL;
>> +		nb8800_tx_dma_start(dev);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
>> +
>> +	priv->tx_next = next;
>
> Are there strong reasons why nb8800_tx_done could not kick between
> spin_unlock_irqrestore and the non-local update of priv->tx_next ?

Good catch.  priv->tx_next wasn't accessed elsewhere in an earlier
version, and I forgot to fix that.  nb8800_tx_done() makes sure the DMA
has actually finished, so priv->tx_next should be updated before
starting the DMA rather than after.  The check against tx_next in
nb8800_tx_done() is only to put some limit on the loop and to avoid
confusion when nb8800_dma_stop() does it's dance.  There should be no
need for more synchronisation here than what the already present memory
barriers provide.

> [...]
>> +static irqreturn_t nb8800_irq(int irq, void *dev_id)
>> +{
>> +	struct net_device *dev = dev_id;
>> +	struct nb8800_priv *priv = netdev_priv(dev);
>> +	u32 val;
>> +
>> +	/* tx interrupt */
>> +	val = nb8800_readl(priv, NB8800_TXC_SR);
>> +	if (val) {
> [...]
>> +	}
>> +
>> +	/* rx interrupt */
>> +	val = nb8800_readl(priv, NB8800_RXC_SR);
>> +	if (val) {
> [...]
>> +	}
>> +
>> +	return IRQ_HANDLED;
>
> Returning IRQ_HANDLED is fine if one of those hold:
> 1. you're sure that at least one of the "if" branch is used
> 2. you'll be able to quickly figure out what's happening whenever 1. stops
>    being true.

You're right, better to check that the device really did have something
to say.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266926

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 00:40:09 +0000

> When the DMA complete interrupt arrives, the next chain should be
> kicked off as quickly as possible, and I don't see why that would
> benefit from being done in napi context.

NAPI isn't about low latency, it's about fairness and interrupt
mitigation.

You probably don't even realize that all of the TX SKB freeing you do
in the hardware interrupt handler end up being actually processed by a
scheduled software interrupt anyways.

So you are gaining almost nothing by not doing TX completion in NAPI
context, whereas by doing so you would be gaining a lot including
more simplified locking or even the ability to do no locking at all.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267155 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 00:40:09 +0000
>
>> When the DMA complete interrupt arrives, the next chain should be
>> kicked off as quickly as possible, and I don't see why that would
>> benefit from being done in napi context.
>
> NAPI isn't about low latency, it's about fairness and interrupt
> mitigation.
>
> You probably don't even realize that all of the TX SKB freeing you do
> in the hardware interrupt handler end up being actually processed by a
> scheduled software interrupt anyways.
>
> So you are gaining almost nothing by not doing TX completion in NAPI
> context, whereas by doing so you would be gaining a lot including
> more simplified locking or even the ability to do no locking at all.

TX completion is separate from restarting the DMA, and moving that to
NAPI may well be a good idea.  Should I simply napi_schedule() if the
hardware indicates TX is complete and do the cleanup in the NAPI poll
function?

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267180 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

Måns Rullgård <mans@mansr.com> writes:

> David Miller <davem@davemloft.net> writes:
>
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Wed, 11 Nov 2015 00:40:09 +0000
>>
>>> When the DMA complete interrupt arrives, the next chain should be
>>> kicked off as quickly as possible, and I don't see why that would
>>> benefit from being done in napi context.
>>
>> NAPI isn't about low latency, it's about fairness and interrupt
>> mitigation.
>>
>> You probably don't even realize that all of the TX SKB freeing you do
>> in the hardware interrupt handler end up being actually processed by a
>> scheduled software interrupt anyways.
>>
>> So you are gaining almost nothing by not doing TX completion in NAPI
>> context, whereas by doing so you would be gaining a lot including
>> more simplified locking or even the ability to do no locking at all.
>
> TX completion is separate from restarting the DMA, and moving that to
> NAPI may well be a good idea.  Should I simply napi_schedule() if the
> hardware indicates TX is complete and do the cleanup in the NAPI poll
> function?

I tried that, and throughput (as measured by iperf3) dropped by 2%.
Maybe I did something wrong.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267189

On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:

> I tried that, and throughput (as measured by iperf3) dropped by 2%.
> Maybe I did something wrong.

What link speed have you used, what was the throughput you got,
and is the receiver using the same NIC ?




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267199 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:
>
>> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>> Maybe I did something wrong.
>
> What link speed have you used, what was the throughput you got,
> and is the receiver using the same NIC ?

1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
Why does it matter what NIC the receiver has?

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267208

On Wed, 2015-11-11 at 13:48 +0000, Måns Rullgård wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> 
> > On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:
> >
> >> I tried that, and throughput (as measured by iperf3) dropped by 2%.
> >> Maybe I did something wrong.
> >
> > What link speed have you used, what was the throughput you got,
> > and is the receiver using the same NIC ?
> 
> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
> Why does it matter what NIC the receiver has?

Because at 1Gb line rate, you better get GRO properly implemented in the
receiver, so that TCP stack does not send one ACK every 2 MSS.

Send speed is also dependent on the number of ACK packets the sender has
to process.

This is why I suggested you use napi_gro_receive() in your driver.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267216 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Wed, 2015-11-11 at 13:48 +0000, Måns Rullgård wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>> 
>> > On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:
>> >
>> >> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>> >> Maybe I did something wrong.
>> >
>> > What link speed have you used, what was the throughput you got,
>> > and is the receiver using the same NIC ?
>> 
>> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
>> Why does it matter what NIC the receiver has?
>
> Because at 1Gb line rate, you better get GRO properly implemented in the
> receiver, so that TCP stack does not send one ACK every 2 MSS.
>
> Send speed is also dependent on the number of ACK packets the sender has
> to process.
>
> This is why I suggested you use napi_gro_receive() in your driver.

FWIW, with UDP I get 650 Mbps.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267224 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

Måns Rullgård <mans@mansr.com> writes:

> Eric Dumazet <eric.dumazet@gmail.com> writes:
>
>> On Wed, 2015-11-11 at 13:48 +0000, Måns Rullgård wrote:
>>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>>> 
>>> > On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:
>>> >
>>> >> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>>> >> Maybe I did something wrong.
>>> >
>>> > What link speed have you used, what was the throughput you got,
>>> > and is the receiver using the same NIC ?
>>> 
>>> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
>>> Why does it matter what NIC the receiver has?
>>
>> Because at 1Gb line rate, you better get GRO properly implemented in the
>> receiver, so that TCP stack does not send one ACK every 2 MSS.
>>
>> Send speed is also dependent on the number of ACK packets the sender has
>> to process.
>>
>> This is why I suggested you use napi_gro_receive() in your driver.
>
> FWIW, with UDP I get 650 Mbps.

It seems the on-chip interconnect is limiting memory bandwidth available
to the Ethernet DMA.  If I increase the limits, I get 800 Mbps over TCP
and 850 Mbps over UDP with the driver posted here, the TCP case being
CPU bound in csum_partial_copy_from_user.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267229

On Wed, 2015-11-11 at 14:35 +0000, Måns Rullgård wrote:

> It seems the on-chip interconnect is limiting memory bandwidth available
> to the Ethernet DMA.  If I increase the limits, I get 800 Mbps over TCP
> and 850 Mbps over UDP with the driver posted here, the TCP case being
> CPU bound in csum_partial_copy_from_user.

Ah right, NIC has no TX checksum offload :(



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267227

On Wed, 2015-11-11 at 14:15 +0000, Måns Rullgård wrote:

> FWIW, with UDP I get 650 Mbps.

Have you tried pktgen ? It should give you raw numbers, without the
stack overhead, especially when using the clone operator.

(It also has xmit_more support)

If the NIC can not get more than 650 Mbps, not worth trying implementing
tso in your driver (using net/core/tso.c helpers)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267297

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 13:04:07 +0000

> Måns Rullgård <mans@mansr.com> writes:
> 
>> David Miller <davem@davemloft.net> writes:
>>
>>> From: Måns Rullgård <mans@mansr.com>
>>> Date: Wed, 11 Nov 2015 00:40:09 +0000
>>>
>>>> When the DMA complete interrupt arrives, the next chain should be
>>>> kicked off as quickly as possible, and I don't see why that would
>>>> benefit from being done in napi context.
>>>
>>> NAPI isn't about low latency, it's about fairness and interrupt
>>> mitigation.
>>>
>>> You probably don't even realize that all of the TX SKB freeing you do
>>> in the hardware interrupt handler end up being actually processed by a
>>> scheduled software interrupt anyways.
>>>
>>> So you are gaining almost nothing by not doing TX completion in NAPI
>>> context, whereas by doing so you would be gaining a lot including
>>> more simplified locking or even the ability to do no locking at all.
>>
>> TX completion is separate from restarting the DMA, and moving that to
>> NAPI may well be a good idea.  Should I simply napi_schedule() if the
>> hardware indicates TX is complete and do the cleanup in the NAPI poll
>> function?
> 
> I tried that, and throughput (as measured by iperf3) dropped by 2%.
> Maybe I did something wrong.

Did you fix all the locking in that change?

Since all of your TX handling runs in software interrupt context, you
can stop using IRQ locking and use BH locking driver-wide instead.

And actually, no locking is really needed for TX processing.  With
proper memory barriers and properly crafter queue state tests, you
can run completely lockless.

Again, look at example drivers.  I know, for example, that
drivers/net/ethernet/broadcom/tg3.c runs TX lockless.  You'll
see that tg3_tx() takes no locks at all.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267370 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 13:04:07 +0000
>
>> Måns Rullgård <mans@mansr.com> writes:
>> 
>>> David Miller <davem@davemloft.net> writes:
>>>
>>>> From: Måns Rullgård <mans@mansr.com>
>>>> Date: Wed, 11 Nov 2015 00:40:09 +0000
>>>>
>>>>> When the DMA complete interrupt arrives, the next chain should be
>>>>> kicked off as quickly as possible, and I don't see why that would
>>>>> benefit from being done in napi context.
>>>>
>>>> NAPI isn't about low latency, it's about fairness and interrupt
>>>> mitigation.
>>>>
>>>> You probably don't even realize that all of the TX SKB freeing you do
>>>> in the hardware interrupt handler end up being actually processed by a
>>>> scheduled software interrupt anyways.
>>>>
>>>> So you are gaining almost nothing by not doing TX completion in NAPI
>>>> context, whereas by doing so you would be gaining a lot including
>>>> more simplified locking or even the ability to do no locking at all.
>>>
>>> TX completion is separate from restarting the DMA, and moving that to
>>> NAPI may well be a good idea.  Should I simply napi_schedule() if the
>>> hardware indicates TX is complete and do the cleanup in the NAPI poll
>>> function?
>> 
>> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>> Maybe I did something wrong.
>
> Did you fix all the locking in that change?
>
> Since all of your TX handling runs in software interrupt context, you
> can stop using IRQ locking and use BH locking driver-wide instead.
>
> And actually, no locking is really needed for TX processing.  With
> proper memory barriers and properly crafter queue state tests, you
> can run completely lockless.
>
> Again, look at example drivers.  I know, for example, that
> drivers/net/ethernet/broadcom/tg3.c runs TX lockless.  You'll
> see that tg3_tx() takes no locks at all.

The way the hardware works, once a DMA operation has been started,
adding more packets to the active chain can't be done reliably.  For
that reason, if start_xmit is called (with xmit_more zero) while a DMA
operation is in progress, the new packet(s) must be queued until the
hardware raises the DMA complete interrupt.  At that time, the next
pending DMA chain, if any, can be kicked off.  If the TX DMA channel is
idle when start_xmit is called, it can be started immediately.  Checking
the DMA status and starting it if idle has to be done atomically
somehow.

There is a separate indication for actual TX completion, and the
interrupt for that can be set to only fire every 7 frames or when a
timeout expires.  When this happens, the TX cleanup needs to run, and
that can obviously be done from NAPI without using any locks.

Bear in mind that this hardware is quite primitive compared to modern
high-performance Ethernet controllers from the likes of Intel and
Broadcom.  The documentation I have is dated 2003.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267391 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 18:25:05 +0000
>
>> If the TX DMA channel is idle when start_xmit is called, it can be
>> started immediately.  Checking the DMA status and starting it if
>> idle has to be done atomically somehow.
>
> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
> by the TX queue spinlock.

Yes, but the DMA needs to be restarted from some other context if it was
busy when start_xmit checked.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267399

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 19:09:19 +0000

> David Miller <davem@davemloft.net> writes:
> 
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>
>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>> started immediately.  Checking the DMA status and starting it if
>>> idle has to be done atomically somehow.
>>
>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>> by the TX queue spinlock.
> 
> Yes, but the DMA needs to be restarted from some other context if it was
> busy when start_xmit checked.

Then you can probably use the TXQ lock in the interrupt handler just for
that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267400 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 19:09:19 +0000
>
>> David Miller <davem@davemloft.net> writes:
>> 
>>> From: Måns Rullgård <mans@mansr.com>
>>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>>
>>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>>> started immediately.  Checking the DMA status and starting it if
>>>> idle has to be done atomically somehow.
>>>
>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>> by the TX queue spinlock.
>> 
>> Yes, but the DMA needs to be restarted from some other context if it was
>> busy when start_xmit checked.
>
> Then you can probably use the TXQ lock in the interrupt handler just for
> that.

That seems a bit heavy-handed when the critical section for this is only
a tiny part of the start_xmit function.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267405

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 19:17:07 +0000

> David Miller <davem@davemloft.net> writes:
> 
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Wed, 11 Nov 2015 19:09:19 +0000
>>
>>> David Miller <davem@davemloft.net> writes:
>>> 
>>>> From: Måns Rullgård <mans@mansr.com>
>>>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>>>
>>>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>>>> started immediately.  Checking the DMA status and starting it if
>>>>> idle has to be done atomically somehow.
>>>>
>>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>>> by the TX queue spinlock.
>>> 
>>> Yes, but the DMA needs to be restarted from some other context if it was
>>> busy when start_xmit checked.
>>
>> Then you can probably use the TXQ lock in the interrupt handler just for
>> that.
> 
> That seems a bit heavy-handed when the critical section for this is only
> a tiny part of the start_xmit function.

Then what synchornization primitive other than spin locks are you going
to use for this?

My point is that there is a spinlock the core code is _already_ taking,
unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
take advantage of that rather than using another lock of your own.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267406

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 19:25:46 +0000

> David Miller <davem@davemloft.net> writes:
> 
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Wed, 11 Nov 2015 19:17:07 +0000
>>
>>> David Miller <davem@davemloft.net> writes:
>>> 
>>>> From: Måns Rullgård <mans@mansr.com>
>>>> Date: Wed, 11 Nov 2015 19:09:19 +0000
>>>>
>>>>> David Miller <davem@davemloft.net> writes:
>>>>> 
>>>>>> From: Måns Rullgård <mans@mansr.com>
>>>>>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>>>>>
>>>>>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>>>>>> started immediately.  Checking the DMA status and starting it if
>>>>>>> idle has to be done atomically somehow.
>>>>>>
>>>>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>>>>> by the TX queue spinlock.
>>>>> 
>>>>> Yes, but the DMA needs to be restarted from some other context if it was
>>>>> busy when start_xmit checked.
>>>>
>>>> Then you can probably use the TXQ lock in the interrupt handler just for
>>>> that.
>>> 
>>> That seems a bit heavy-handed when the critical section for this is only
>>> a tiny part of the start_xmit function.
>>
>> Then what synchornization primitive other than spin locks are you going
>> to use for this?
>>
>> My point is that there is a spinlock the core code is _already_ taking,
>> unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
>> take advantage of that rather than using another lock of your own.
> 
> I get that.  But that remains locked for the duration of ndo_start_xmit()
> whereas the part that needs to be synchronised with the DMA completion
> IRQ handler is tiny.  Having the IRQ handler spin for the duration of
> ndo_start_xmit() seemed silly to me.

I don't think it's silly at all.

And unless you can measure it making a difference, don't knock the idea.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267408 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 19:25:46 +0000
>
>> David Miller <davem@davemloft.net> writes:
>> 
>>> From: Måns Rullgård <mans@mansr.com>
>>> Date: Wed, 11 Nov 2015 19:17:07 +0000
>>>
>>>> David Miller <davem@davemloft.net> writes:
>>>> 
>>>>> From: Måns Rullgård <mans@mansr.com>
>>>>> Date: Wed, 11 Nov 2015 19:09:19 +0000
>>>>>
>>>>>> David Miller <davem@davemloft.net> writes:
>>>>>> 
>>>>>>> From: Måns Rullgård <mans@mansr.com>
>>>>>>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>>>>>>
>>>>>>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>>>>>>> started immediately.  Checking the DMA status and starting it if
>>>>>>>> idle has to be done atomically somehow.
>>>>>>>
>>>>>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>>>>>> by the TX queue spinlock.
>>>>>> 
>>>>>> Yes, but the DMA needs to be restarted from some other context if it was
>>>>>> busy when start_xmit checked.
>>>>>
>>>>> Then you can probably use the TXQ lock in the interrupt handler just for
>>>>> that.
>>>> 
>>>> That seems a bit heavy-handed when the critical section for this is only
>>>> a tiny part of the start_xmit function.
>>>
>>> Then what synchornization primitive other than spin locks are you going
>>> to use for this?
>>>
>>> My point is that there is a spinlock the core code is _already_ taking,
>>> unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
>>> take advantage of that rather than using another lock of your own.
>> 
>> I get that.  But that remains locked for the duration of ndo_start_xmit()
>> whereas the part that needs to be synchronised with the DMA completion
>> IRQ handler is tiny.  Having the IRQ handler spin for the duration of
>> ndo_start_xmit() seemed silly to me.
>
> I don't think it's silly at all.

I'm sure I read somewhere that the time spent spinning on a lock should
be kept as small as possible.

> And unless you can measure it making a difference, don't knock the idea.

I tried using netif_tx_lock() in the IRQ handler instead, and it locked
up solid.  Clearly that was the wrong thing to do.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web