* [PATCH RFT net-next 0/4] via-rhine receive buffers rework
@ 2015-04-06 18:01 Francois Romieu
2015-04-06 18:01 ` [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update Francois Romieu
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Francois Romieu @ 2015-04-06 18:01 UTC (permalink / raw)
To: netdev; +Cc: Nix, David S. Miller, rl, Bjarke Istrup Pedersen
This is an hopefully readable rework of my patch from two days ago.
It's different enough that it imho requires someone's Tested-by.
Patches #1..#3 are mostly there to slim patch #4. Patch #1 may make
some sense as a standalone fix though.
The series applies without error against v3.19 - what Nix uses - and
davem-next as of 5888b93b750609680735d6b8b737703083ef40ff ("Merge branch
'nf-hook-compress'").
Please test and review.
Francois Romieu (4):
via-rhine: commit receive buffer address before descriptor status update.
via-rhine: add allocation helpers.
via-rhine: gotoize rhine_open error path.
via-rhine: forbid holes in the receive descriptor ring.
drivers/net/ethernet/via/via-rhine.c | 163 +++++++++++++++++++++--------------
1 file changed, 100 insertions(+), 63 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-06 18:01 [PATCH RFT net-next 0/4] via-rhine receive buffers rework Francois Romieu
@ 2015-04-06 18:01 ` Francois Romieu
2015-04-07 19:52 ` David Miller
2015-04-07 19:54 ` Alexander Duyck
2015-04-06 18:01 ` [PATCH net-next 2/4] via-rhine: add allocation helpers Francois Romieu
` (3 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Francois Romieu @ 2015-04-06 18:01 UTC (permalink / raw)
To: netdev; +Cc: Nix, David S. Miller, rl, Bjarke Istrup Pedersen
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
drivers/net/ethernet/via/via-rhine.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index a191afc..00fea3d 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit)
break;
}
rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
+ wmb();
}
rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn);
}
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH net-next 2/4] via-rhine: add allocation helpers.
2015-04-06 18:01 [PATCH RFT net-next 0/4] via-rhine receive buffers rework Francois Romieu
2015-04-06 18:01 ` [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update Francois Romieu
@ 2015-04-06 18:01 ` Francois Romieu
2015-04-06 18:01 ` [PATCH net-next 3/4] via-rhine: gotoize rhine_open error path Francois Romieu
` (2 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Francois Romieu @ 2015-04-06 18:01 UTC (permalink / raw)
To: netdev; +Cc: Nix, David S. Miller, rl, Bjarke Istrup Pedersen
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
drivers/net/ethernet/via/via-rhine.c | 57 +++++++++++++++++++++++++++---------
1 file changed, 43 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 00fea3d..cf8dfcd 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1211,12 +1211,47 @@ static void free_ring(struct net_device* dev)
}
-static void alloc_rbufs(struct net_device *dev)
+struct rhine_skb_dma {
+ struct sk_buff *skb;
+ dma_addr_t dma;
+};
+
+static inline int rhine_skb_dma_init(struct net_device *dev,
+ struct rhine_skb_dma *sd)
{
struct rhine_private *rp = netdev_priv(dev);
struct device *hwdev = dev->dev.parent;
+ const int size = rp->rx_buf_sz;
+
+ sd->skb = netdev_alloc_skb(dev, size);
+ if (!sd->skb)
+ return -ENOMEM;
+
+ sd->dma = dma_map_single(hwdev, sd->skb->data, size, DMA_FROM_DEVICE);
+ if (unlikely(dma_mapping_error(hwdev, sd->dma))) {
+ netif_err(rp, drv, dev, "Rx DMA mapping failure\n");
+ dev_kfree_skb_any(sd->skb);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static inline void rhine_skb_dma_nic_store(struct rhine_private *rp,
+ struct rhine_skb_dma *sd, int entry)
+{
+ rp->rx_skbuff_dma[entry] = sd->dma;
+ rp->rx_skbuff[entry] = sd->skb;
+
+ rp->rx_ring[entry].addr = cpu_to_le32(sd->dma);
+ wmb();
+}
+
+static void alloc_rbufs(struct net_device *dev)
+{
+ struct rhine_private *rp = netdev_priv(dev);
dma_addr_t next;
- int i;
+ int rc, i;
rp->dirty_rx = rp->cur_rx = 0;
@@ -1237,20 +1272,14 @@ static void alloc_rbufs(struct net_device *dev)
/* Fill in the Rx buffers. Handle allocation failure gracefully. */
for (i = 0; i < RX_RING_SIZE; i++) {
- struct sk_buff *skb = netdev_alloc_skb(dev, rp->rx_buf_sz);
- rp->rx_skbuff[i] = skb;
- if (skb == NULL)
- break;
+ struct rhine_skb_dma sd;
- rp->rx_skbuff_dma[i] =
- dma_map_single(hwdev, skb->data, rp->rx_buf_sz,
- DMA_FROM_DEVICE);
- if (dma_mapping_error(hwdev, rp->rx_skbuff_dma[i])) {
- rp->rx_skbuff_dma[i] = 0;
- dev_kfree_skb(skb);
+ rc = rhine_skb_dma_init(dev, &sd);
+ if (rc < 0)
break;
- }
- rp->rx_ring[i].addr = cpu_to_le32(rp->rx_skbuff_dma[i]);
+
+ rhine_skb_dma_nic_store(rp, &sd, i);
+
rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
}
rp->dirty_rx = (unsigned int)(i - RX_RING_SIZE);
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH net-next 3/4] via-rhine: gotoize rhine_open error path.
2015-04-06 18:01 [PATCH RFT net-next 0/4] via-rhine receive buffers rework Francois Romieu
2015-04-06 18:01 ` [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update Francois Romieu
2015-04-06 18:01 ` [PATCH net-next 2/4] via-rhine: add allocation helpers Francois Romieu
@ 2015-04-06 18:01 ` Francois Romieu
2015-04-06 18:01 ` [PATCH net-next 4/4] via-rhine: forbid holes in the receive descriptor ring Francois Romieu
2015-04-06 21:46 ` [PATCH RFT net-next 0/4] via-rhine receive buffers rework Nix
4 siblings, 0 replies; 19+ messages in thread
From: Francois Romieu @ 2015-04-06 18:01 UTC (permalink / raw)
To: netdev; +Cc: Nix, David S. Miller, rl, Bjarke Istrup Pedersen
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
drivers/net/ethernet/via/via-rhine.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index cf8dfcd..ae1ab6b 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1684,15 +1684,14 @@ static int rhine_open(struct net_device *dev)
rc = request_irq(rp->irq, rhine_interrupt, IRQF_SHARED, dev->name, dev);
if (rc)
- return rc;
+ goto out;
netif_dbg(rp, ifup, dev, "%s() irq %d\n", __func__, rp->irq);
rc = alloc_ring(dev);
- if (rc) {
- free_irq(rp->irq, dev);
- return rc;
- }
+ if (rc < 0)
+ goto out_free_irq;
+
alloc_rbufs(dev);
alloc_tbufs(dev);
rhine_chip_reset(dev);
@@ -1705,7 +1704,12 @@ static int rhine_open(struct net_device *dev)
netif_start_queue(dev);
- return 0;
+out:
+ return rc;
+
+out_free_irq:
+ free_irq(rp->irq, dev);
+ goto out;
}
static void rhine_reset_task(struct work_struct *work)
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH net-next 4/4] via-rhine: forbid holes in the receive descriptor ring.
2015-04-06 18:01 [PATCH RFT net-next 0/4] via-rhine receive buffers rework Francois Romieu
` (2 preceding siblings ...)
2015-04-06 18:01 ` [PATCH net-next 3/4] via-rhine: gotoize rhine_open error path Francois Romieu
@ 2015-04-06 18:01 ` Francois Romieu
2015-04-06 21:46 ` [PATCH RFT net-next 0/4] via-rhine receive buffers rework Nix
4 siblings, 0 replies; 19+ messages in thread
From: Francois Romieu @ 2015-04-06 18:01 UTC (permalink / raw)
To: netdev; +Cc: Nix, David S. Miller, rl, Bjarke Istrup Pedersen
Rationales:
- throttle work under memory pressure;
- lower receive descriptor recycling latency for the network adapter;
- lower the maintenance burden of uncommon paths.
The patch is twofold:
1. it fails early if the receive buffer ring can't be completely
initialized at dev->open() time
2. it drops packets on the floor in the napi receive handler so as to
keep the ring full.
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
drivers/net/ethernet/via/via-rhine.c | 101 ++++++++++++++++++-----------------
1 file changed, 52 insertions(+), 49 deletions(-)
diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index ae1ab6b..94729b1 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -471,7 +471,7 @@ struct rhine_private {
/* Frequently used values: keep some adjacent for cache effect. */
u32 quirks;
struct rx_desc *rx_head_desc;
- unsigned int cur_rx, dirty_rx; /* Producer/consumer ring indices */
+ unsigned int cur_rx;
unsigned int cur_tx, dirty_tx;
unsigned int rx_buf_sz; /* Based on MTU+slack. */
struct rhine_stats rx_stats;
@@ -1237,6 +1237,16 @@ static inline int rhine_skb_dma_init(struct net_device *dev,
return 0;
}
+static void rhine_reset_rbufs(struct rhine_private *rp)
+{
+ int i;
+
+ rp->cur_rx = 0;
+
+ for (i = 0; i < RX_RING_SIZE; i++)
+ rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
+}
+
static inline void rhine_skb_dma_nic_store(struct rhine_private *rp,
struct rhine_skb_dma *sd, int entry)
{
@@ -1247,14 +1257,14 @@ static inline void rhine_skb_dma_nic_store(struct rhine_private *rp,
wmb();
}
-static void alloc_rbufs(struct net_device *dev)
+static void free_rbufs(struct net_device* dev);
+
+static int alloc_rbufs(struct net_device *dev)
{
struct rhine_private *rp = netdev_priv(dev);
dma_addr_t next;
int rc, i;
- rp->dirty_rx = rp->cur_rx = 0;
-
rp->rx_buf_sz = (dev->mtu <= 1500 ? PKT_BUF_SZ : dev->mtu + 32);
rp->rx_head_desc = &rp->rx_ring[0];
next = rp->rx_ring_dma;
@@ -1275,14 +1285,17 @@ static void alloc_rbufs(struct net_device *dev)
struct rhine_skb_dma sd;
rc = rhine_skb_dma_init(dev, &sd);
- if (rc < 0)
- break;
+ if (rc < 0) {
+ free_rbufs(dev);
+ goto out;
+ }
rhine_skb_dma_nic_store(rp, &sd, i);
-
- rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
}
- rp->dirty_rx = (unsigned int)(i - RX_RING_SIZE);
+
+ rhine_reset_rbufs(rp);
+out:
+ return rc;
}
static void free_rbufs(struct net_device* dev)
@@ -1692,7 +1705,10 @@ static int rhine_open(struct net_device *dev)
if (rc < 0)
goto out_free_irq;
- alloc_rbufs(dev);
+ rc = alloc_rbufs(dev);
+ if (rc < 0)
+ goto out_free_ring;
+
alloc_tbufs(dev);
rhine_chip_reset(dev);
rhine_task_enable(rp);
@@ -1707,6 +1723,8 @@ static int rhine_open(struct net_device *dev)
out:
return rc;
+out_free_ring:
+ free_ring(dev);
out_free_irq:
free_irq(rp->irq, dev);
goto out;
@@ -1729,9 +1747,9 @@ static void rhine_reset_task(struct work_struct *work)
/* clear all descriptors */
free_tbufs(dev);
- free_rbufs(dev);
alloc_tbufs(dev);
- alloc_rbufs(dev);
+
+ rhine_reset_rbufs(rp);
/* Reinitialize the hardware. */
rhine_chip_reset(dev);
@@ -2021,16 +2039,18 @@ static int rhine_rx(struct net_device *dev, int limit)
}
}
} else {
- struct sk_buff *skb = NULL;
/* Length should omit the CRC */
int pkt_len = data_size - 4;
+ struct sk_buff *skb;
u16 vlan_tci = 0;
/* Check if the packet is long enough to accept without
copying to a minimally-sized skbuff. */
- if (pkt_len < rx_copybreak)
+ if (pkt_len < rx_copybreak) {
skb = netdev_alloc_skb_ip_align(dev, pkt_len);
- if (skb) {
+ if (unlikely(!skb))
+ goto drop;
+
dma_sync_single_for_cpu(hwdev,
rp->rx_skbuff_dma[entry],
rp->rx_buf_sz,
@@ -2039,25 +2059,28 @@ static int rhine_rx(struct net_device *dev, int limit)
skb_copy_to_linear_data(skb,
rp->rx_skbuff[entry]->data,
pkt_len);
- skb_put(skb, pkt_len);
+
dma_sync_single_for_device(hwdev,
rp->rx_skbuff_dma[entry],
rp->rx_buf_sz,
DMA_FROM_DEVICE);
} else {
+ struct rhine_skb_dma sd;
+
+ if (unlikely(rhine_skb_dma_init(dev, &sd) < 0))
+ goto drop;
+
skb = rp->rx_skbuff[entry];
- if (skb == NULL) {
- netdev_err(dev, "Inconsistent Rx descriptor chain\n");
- break;
- }
- rp->rx_skbuff[entry] = NULL;
- skb_put(skb, pkt_len);
+
dma_unmap_single(hwdev,
rp->rx_skbuff_dma[entry],
rp->rx_buf_sz,
DMA_FROM_DEVICE);
+ rhine_skb_dma_nic_store(rp, &sd, entry);
}
+ skb_put(skb, pkt_len);
+
if (unlikely(desc_length & DescTag))
vlan_tci = rhine_get_vlan_tci(skb, data_size);
@@ -2072,36 +2095,17 @@ static int rhine_rx(struct net_device *dev, int limit)
rp->rx_stats.packets++;
u64_stats_update_end(&rp->rx_stats.syncp);
}
+give_descriptor_to_nic:
+ rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn);
entry = (++rp->cur_rx) % RX_RING_SIZE;
rp->rx_head_desc = &rp->rx_ring[entry];
}
- /* Refill the Rx ring buffers. */
- for (; rp->cur_rx - rp->dirty_rx > 0; rp->dirty_rx++) {
- struct sk_buff *skb;
- entry = rp->dirty_rx % RX_RING_SIZE;
- if (rp->rx_skbuff[entry] == NULL) {
- skb = netdev_alloc_skb(dev, rp->rx_buf_sz);
- rp->rx_skbuff[entry] = skb;
- if (skb == NULL)
- break; /* Better luck next round. */
- rp->rx_skbuff_dma[entry] =
- dma_map_single(hwdev, skb->data,
- rp->rx_buf_sz,
- DMA_FROM_DEVICE);
- if (dma_mapping_error(hwdev,
- rp->rx_skbuff_dma[entry])) {
- dev_kfree_skb(skb);
- rp->rx_skbuff_dma[entry] = 0;
- break;
- }
- rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
- wmb();
- }
- rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn);
- }
-
return count;
+
+drop:
+ dev->stats.rx_dropped++;
+ goto give_descriptor_to_nic;
}
static void rhine_restart_tx(struct net_device *dev) {
@@ -2506,9 +2510,8 @@ static int rhine_resume(struct device *device)
enable_mmio(rp->pioaddr, rp->quirks);
rhine_power_init(dev);
free_tbufs(dev);
- free_rbufs(dev);
alloc_tbufs(dev);
- alloc_rbufs(dev);
+ rhine_reset_rbufs(rp);
rhine_task_enable(rp);
spin_lock_bh(&rp->lock);
init_registers(dev);
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH RFT net-next 0/4] via-rhine receive buffers rework
2015-04-06 18:01 [PATCH RFT net-next 0/4] via-rhine receive buffers rework Francois Romieu
` (3 preceding siblings ...)
2015-04-06 18:01 ` [PATCH net-next 4/4] via-rhine: forbid holes in the receive descriptor ring Francois Romieu
@ 2015-04-06 21:46 ` Nix
2015-04-07 10:46 ` Nix
4 siblings, 1 reply; 19+ messages in thread
From: Nix @ 2015-04-06 21:46 UTC (permalink / raw)
To: Francois Romieu; +Cc: netdev, David S. Miller, rl, Bjarke Istrup Pedersen
On 6 Apr 2015, Francois Romieu outgrape:
> This is an hopefully readable rework of my patch from two days ago.
> It's different enough that it imho requires someone's Tested-by.
I'll give it a torturing tomorrow.
--
NULL && (void)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFT net-next 0/4] via-rhine receive buffers rework
2015-04-06 21:46 ` [PATCH RFT net-next 0/4] via-rhine receive buffers rework Nix
@ 2015-04-07 10:46 ` Nix
2015-04-07 19:31 ` Francois Romieu
0 siblings, 1 reply; 19+ messages in thread
From: Nix @ 2015-04-07 10:46 UTC (permalink / raw)
To: Francois Romieu; +Cc: netdev, David S. Miller, rl, Bjarke Istrup Pedersen
On 6 Apr 2015, nix@esperi.org.uk verbalised:
> On 6 Apr 2015, Francois Romieu outgrape:
>
>> This is an hopefully readable rework of my patch from two days ago.
>> It's different enough that it imho requires someone's Tested-by.
>
> I'll give it a torturing tomorrow.
Tormented by routing a simultaneous iperf and make-over-nfs through it
(with the make using ccache, and the ccache cache directory *also*
accessed via nfs, to hit it really hard :) ).
No crashes in several hours of running: looks good!
--
NULL && (void)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFT net-next 0/4] via-rhine receive buffers rework
2015-04-07 10:46 ` Nix
@ 2015-04-07 19:31 ` Francois Romieu
2015-04-07 19:41 ` Nix
0 siblings, 1 reply; 19+ messages in thread
From: Francois Romieu @ 2015-04-07 19:31 UTC (permalink / raw)
To: Nix; +Cc: netdev, David S. Miller, rl, Bjarke Istrup Pedersen
Nix <nix@esperi.org.uk> :
[...]
> No crashes in several hours of running: looks good!
Mildly. :o/
You may never notice if you don't resume and the reset work queue doesn't
run but patch #4 lacks the bits below.
(rx_head_desc should go away)
diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index a54a9f4..7f31c3a 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1244,6 +1244,7 @@ static void rhine_reset_rbufs(struct rhine_private *rp)
int i;
rp->cur_rx = 0;
+ rp->rx_head_desc = rp->rx_ring;
for (i = 0; i < RX_RING_SIZE; i++)
rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
@@ -1268,7 +1269,6 @@ static int alloc_rbufs(struct net_device *dev)
int rc, i;
rp->rx_buf_sz = (dev->mtu <= 1500 ? PKT_BUF_SZ : dev->mtu + 32);
- rp->rx_head_desc = &rp->rx_ring[0];
next = rp->rx_ring_dma;
/* Init the ring entries */
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH RFT net-next 0/4] via-rhine receive buffers rework
2015-04-07 19:31 ` Francois Romieu
@ 2015-04-07 19:41 ` Nix
0 siblings, 0 replies; 19+ messages in thread
From: Nix @ 2015-04-07 19:41 UTC (permalink / raw)
To: Francois Romieu; +Cc: netdev, David S. Miller, rl, Bjarke Istrup Pedersen
On 7 Apr 2015, Francois Romieu stated:
> Nix <nix@esperi.org.uk> :
> [...]
>> No crashes in several hours of running: looks good!
>
> Mildly. :o/
>
> You may never notice if you don't resume and the reset work queue doesn't
> run but patch #4 lacks the bits below.
I don't suspend this box, no, it uses only 5W of power and it's my link
to the internet so suspending it would be a bad move :)
> (rx_head_desc should go away)
I'll test this out on top of the other stuff tomorrow...
--
NULL && (void)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-06 18:01 ` [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update Francois Romieu
@ 2015-04-07 19:52 ` David Miller
2015-04-07 21:02 ` Francois Romieu
2015-04-07 19:54 ` Alexander Duyck
1 sibling, 1 reply; 19+ messages in thread
From: David Miller @ 2015-04-07 19:52 UTC (permalink / raw)
To: romieu; +Cc: netdev, nix, rl, gurligebis
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Mon, 6 Apr 2015 20:01:49 +0200
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> ---
> drivers/net/ethernet/via/via-rhine.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
> index a191afc..00fea3d 100644
> --- a/drivers/net/ethernet/via/via-rhine.c
> +++ b/drivers/net/ethernet/via/via-rhine.c
> @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit)
> break;
> }
> rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
> + wmb();
dma_wmb() perhaps? I think this is exactly the situation that interface was
added for.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-06 18:01 ` [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update Francois Romieu
2015-04-07 19:52 ` David Miller
@ 2015-04-07 19:54 ` Alexander Duyck
1 sibling, 0 replies; 19+ messages in thread
From: Alexander Duyck @ 2015-04-07 19:54 UTC (permalink / raw)
To: Francois Romieu, netdev; +Cc: Nix, David S. Miller, rl, Bjarke Istrup Pedersen
On 04/06/2015 11:01 AM, Francois Romieu wrote:
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> ---
> drivers/net/ethernet/via/via-rhine.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
> index a191afc..00fea3d 100644
> --- a/drivers/net/ethernet/via/via-rhine.c
> +++ b/drivers/net/ethernet/via/via-rhine.c
> @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit)
> break;
> }
> rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
> + wmb();
> }
> rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn);
> }
Do you need a full wmb() here or would a dma_wmb() be enough? It could
make a difference on some processor architectures.
- Alex
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-07 19:52 ` David Miller
@ 2015-04-07 21:02 ` Francois Romieu
2015-04-07 21:19 ` Alexander Duyck
2015-04-07 21:27 ` David Miller
0 siblings, 2 replies; 19+ messages in thread
From: Francois Romieu @ 2015-04-07 21:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev, nix, rl, gurligebis, Alexander Duyck
David Miller <davem@davemloft.net> :
> From: Francois Romieu <romieu@fr.zoreil.com>
[...]
> > @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit)
> > break;
> > }
> > rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
> > + wmb();
>
> dma_wmb() perhaps? I think this is exactly the situation that interface was
> added for.
I need the buffer address to be written in the receive descriptor before
the descriptor status is. The cpu does W1, W2 and the nic mustn't see W2, W1.
--
Ueimor
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-07 21:02 ` Francois Romieu
@ 2015-04-07 21:19 ` Alexander Duyck
2015-04-07 21:27 ` David Miller
1 sibling, 0 replies; 19+ messages in thread
From: Alexander Duyck @ 2015-04-07 21:19 UTC (permalink / raw)
To: Francois Romieu, David Miller; +Cc: netdev, nix, rl, gurligebis
On 04/07/2015 02:02 PM, Francois Romieu wrote:
> David Miller <davem@davemloft.net> :
>> From: Francois Romieu <romieu@fr.zoreil.com>
> [...]
>>> @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit)
>>> break;
>>> }
>>> rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
>>> + wmb();
>> dma_wmb() perhaps? I think this is exactly the situation that interface was
>> added for.
> I need the buffer address to be written in the receive descriptor before
> the descriptor status is. The cpu does W1, W2 and the nic mustn't see W2, W1.
That is the point of the dma_wmb(). If you are writing both W1 and W2
to system memory then dma_wmb should be enough, if W1 is system memory
and W2 is device memory (MMIO) then you need wmb().
You can think of dma_wmb as being something similar to smp_wmb w/o the
SMP processor requirement. On architectures that are strong ordered
such as most x86 the wmb() translates to a barrier. On other
architectures it is something usually a little lighter than a full wmb()
so for example on PowerPC wmb is sync() while dma_wmb() is lwsync().
- Alex
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-07 21:02 ` Francois Romieu
2015-04-07 21:19 ` Alexander Duyck
@ 2015-04-07 21:27 ` David Miller
2015-04-07 21:54 ` Eric Dumazet
1 sibling, 1 reply; 19+ messages in thread
From: David Miller @ 2015-04-07 21:27 UTC (permalink / raw)
To: romieu; +Cc: netdev, nix, rl, gurligebis, alexander.h.duyck
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Tue, 7 Apr 2015 23:02:48 +0200
> David Miller <davem@davemloft.net> :
>> From: Francois Romieu <romieu@fr.zoreil.com>
> [...]
>> > @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit)
>> > break;
>> > }
>> > rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
>> > + wmb();
>>
>> dma_wmb() perhaps? I think this is exactly the situation that interface was
>> added for.
>
> I need the buffer address to be written in the receive descriptor before
> the descriptor status is. The cpu does W1, W2 and the nic mustn't see W2, W1.
That's exactly dma_wmb().
It barriers cpu writes so that the device sees things in a certain
order.
It's what all the most common ethernet chip drivers use in their
descriptor handling routines now.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-07 21:27 ` David Miller
@ 2015-04-07 21:54 ` Eric Dumazet
2015-04-07 22:12 ` Alexander Duyck
0 siblings, 1 reply; 19+ messages in thread
From: Eric Dumazet @ 2015-04-07 21:54 UTC (permalink / raw)
To: David Miller; +Cc: romieu, netdev, nix, rl, gurligebis, alexander.h.duyck
On Tue, 2015-04-07 at 17:27 -0400, David Miller wrote:
> That's exactly dma_wmb().
>
> It barriers cpu writes so that the device sees things in a certain
> order.
>
> It's what all the most common ethernet chip drivers use in their
> descriptor handling routines now.
To be fair, only 2 drivers currently use dma_wmb()
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-07 21:54 ` Eric Dumazet
@ 2015-04-07 22:12 ` Alexander Duyck
2015-04-07 22:22 ` Eric Dumazet
0 siblings, 1 reply; 19+ messages in thread
From: Alexander Duyck @ 2015-04-07 22:12 UTC (permalink / raw)
To: Eric Dumazet, David Miller; +Cc: romieu, netdev, nix, rl, gurligebis
On 04/07/2015 02:54 PM, Eric Dumazet wrote:
> On Tue, 2015-04-07 at 17:27 -0400, David Miller wrote:
>
>> That's exactly dma_wmb().
>>
>> It barriers cpu writes so that the device sees things in a certain
>> order.
>>
>> It's what all the most common ethernet chip drivers use in their
>> descriptor handling routines now.
> To be fair, only 2 drivers currently use dma_wmb()
Hey, I got at least 4.. :-) I only got around to patching 3 Intel
drivers and one RealTek since that is what I had to test with. I was
honestly hoping there would be more interest from other developers to
pick this up and update their drivers to avoid unnecessary barriers but
it doesn't look like I have had much luck on that front.
Maybe what I can do is submit a set of patches over the next couple of
weeks to try and update all the spots that either need to have a barrier
added, such as what is being addressed here, or can have a barrier
weakened as I have already done for a few other drivers.
- Alex
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-07 22:12 ` Alexander Duyck
@ 2015-04-07 22:22 ` Eric Dumazet
2015-04-07 23:20 ` Alexander Duyck
0 siblings, 1 reply; 19+ messages in thread
From: Eric Dumazet @ 2015-04-07 22:22 UTC (permalink / raw)
To: Alexander Duyck; +Cc: David Miller, romieu, netdev, nix, rl, gurligebis
On Tue, 2015-04-07 at 15:12 -0700, Alexander Duyck wrote:
> On 04/07/2015 02:54 PM, Eric Dumazet wrote:
> > To be fair, only 2 drivers currently use dma_wmb()
>
> Hey, I got at least 4.. :-) I only got around to patching 3 Intel
> drivers and one RealTek since that is what I had to test with. I was
> honestly hoping there would be more interest from other developers to
> pick this up and update their drivers to avoid unnecessary barriers but
> it doesn't look like I have had much luck on that front.
Strange I see only 2 drivers here, and no Intel ones :
# git grep -l dma_wmb drivers/net
drivers/net/ethernet/amd/xgbe/xgbe-dev.c
drivers/net/ethernet/realtek/r8169.c
I probably missed something ;)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-07 22:22 ` Eric Dumazet
@ 2015-04-07 23:20 ` Alexander Duyck
2015-04-08 11:40 ` David Laight
0 siblings, 1 reply; 19+ messages in thread
From: Alexander Duyck @ 2015-04-07 23:20 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, romieu, netdev, nix, rl, gurligebis
On 04/07/2015 03:22 PM, Eric Dumazet wrote:
> On Tue, 2015-04-07 at 15:12 -0700, Alexander Duyck wrote:
>> On 04/07/2015 02:54 PM, Eric Dumazet wrote:
>>> To be fair, only 2 drivers currently use dma_wmb()
>> Hey, I got at least 4.. :-) I only got around to patching 3 Intel
>> drivers and one RealTek since that is what I had to test with. I was
>> honestly hoping there would be more interest from other developers to
>> pick this up and update their drivers to avoid unnecessary barriers but
>> it doesn't look like I have had much luck on that front.
> Strange I see only 2 drivers here, and no Intel ones :
>
> # git grep -l dma_wmb drivers/net
> drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> drivers/net/ethernet/realtek/r8169.c
>
> I probably missed something ;)
Yeah, I was thinking of the dma_rmb more than the dma_wmb, they are
essentially just two sides to the same coin.
dma_rmb - read descriptor status for ownership, then dma_rmb, then if we
own it process rest of descriptor
dma_wmb - write descriptor fields, then dma_wmb, then update status bit
to release ownership
A wmb is still needed if a PIO doorbell must be used to notify the
device of additional descriptors.
- Alex
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update.
2015-04-07 23:20 ` Alexander Duyck
@ 2015-04-08 11:40 ` David Laight
0 siblings, 0 replies; 19+ messages in thread
From: David Laight @ 2015-04-08 11:40 UTC (permalink / raw)
To: 'Alexander Duyck', Eric Dumazet
Cc: David Miller, romieu, netdev, nix, rl, gurligebis
From: Alexander Duyck
> Sent: 08 April 2015 00:21
...
> dma_wmb - write descriptor fields, then dma_wmb, then update status bit
> to release ownership
>
> A wmb is still needed if a PIO doorbell must be used to notify the
> device of additional descriptors.
That is needed after the write that releases ownership.
So you are likely to need a dma_wmb() before the status bit update
and a full wmb() just after it.
If you are adding a lot of frames then you should be able to update
the status for the first fragment last - and only need the barriers
on the final update.
David
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-04-08 11:41 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-06 18:01 [PATCH RFT net-next 0/4] via-rhine receive buffers rework Francois Romieu
2015-04-06 18:01 ` [PATCH net-next 1/4] via-rhine: commit receive buffer address before descriptor status update Francois Romieu
2015-04-07 19:52 ` David Miller
2015-04-07 21:02 ` Francois Romieu
2015-04-07 21:19 ` Alexander Duyck
2015-04-07 21:27 ` David Miller
2015-04-07 21:54 ` Eric Dumazet
2015-04-07 22:12 ` Alexander Duyck
2015-04-07 22:22 ` Eric Dumazet
2015-04-07 23:20 ` Alexander Duyck
2015-04-08 11:40 ` David Laight
2015-04-07 19:54 ` Alexander Duyck
2015-04-06 18:01 ` [PATCH net-next 2/4] via-rhine: add allocation helpers Francois Romieu
2015-04-06 18:01 ` [PATCH net-next 3/4] via-rhine: gotoize rhine_open error path Francois Romieu
2015-04-06 18:01 ` [PATCH net-next 4/4] via-rhine: forbid holes in the receive descriptor ring Francois Romieu
2015-04-06 21:46 ` [PATCH RFT net-next 0/4] via-rhine receive buffers rework Nix
2015-04-07 10:46 ` Nix
2015-04-07 19:31 ` Francois Romieu
2015-04-07 19:41 ` Nix
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).