On Thursday 11 December 2014 19:42:27 Ding Tianhong wrote:
> v9:
> - There is no tx completion interrupts to free DMAd Tx packets, it means taht
> we rely on new tx packets arriving to run the destructors of completed packets,
> which open up space in their sockets's send queues. Sometimes we don't get such
> new packets causing Tx to stall, a single UDP transmitter is a good example of
> this situation, so we need a clean up workqueue to reclaims completed packets,
> the workqueue will only free the last packets which is already stay for several jiffies.
> Also fix some format cleanups.
You must have missed the reply in which David Miller explained why
you can't call skb_orphan and rely on an occasional cleanup of the
queue. Please use something like the patch below:
- drop the broken skb_orphan call
- drop the workqueue
- batch cleanup based on tx_coalesce_frames/usecs for better throughput
- use a reasonable default tx timeout (200us, could be shorted
based on measurements) with a range timer
- fix napi poll function return value
- use a lockless queue for cleanup
Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
Feel free to fold this into your patch rather than keeping it separate.
Completely untested, probably contains some bugs. Please ask if you
have questions about this.
diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 9d37b670db6a..d85c5287654e 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -12,6 +12,7 @@
#include <linux/etherdevice.h>
#include <linux/platform_device.h>
#include <linux/interrupt.h>
+#include <linux/ktime.h>
#include <linux/of_address.h>
#include <linux/phy.h>
#include <linux/of_mdio.h>
@@ -157,9 +158,11 @@ struct hip04_priv {
struct sk_buff *tx_skb[TX_DESC_NUM];
dma_addr_t tx_phys[TX_DESC_NUM];
unsigned int tx_head;
- unsigned int tx_tail;
- int tx_count;
- unsigned long last_tx;
+
+ /* FIXME: make these adjustable through ethtool */
+ int tx_coalesce_frames;
+ int tx_coalesce_usecs;
+ struct hrtimer tx_coalesce_timer;
unsigned char *rx_buf[RX_DESC_NUM];
dma_addr_t rx_phys[RX_DESC_NUM];
@@ -171,10 +174,15 @@ struct hip04_priv {
struct regmap *map;
struct work_struct tx_timeout_task;
- struct workqueue_struct *wq;
- struct delayed_work tx_clean_task;
+ /* written only by tx cleanup */
+ unsigned int tx_tail ____cacheline_aligned_in_smp;
};
+static inline unsigned int tx_count(unsigned int head, unsigned int tail)
+{
+ return (head - tail) % (TX_DESC_NUM - 1);
+}
+
static void hip04_config_port(struct net_device *ndev, u32 speed, u32 duplex)
{
struct hip04_priv *priv = netdev_priv(ndev);
@@ -351,18 +359,21 @@ static int hip04_set_mac_address(struct net_device *ndev, void *addr)
return 0;
}
-static void hip04_tx_reclaim(struct net_device *ndev, bool force)
+static int hip04_tx_reclaim(struct net_device *ndev, bool force)
{
struct hip04_priv *priv = netdev_priv(ndev);
unsigned tx_tail = priv->tx_tail;
struct tx_desc *desc;
unsigned int bytes_compl = 0, pkts_compl = 0;
+ unsigned int count;
- if (priv->tx_count == 0)
+ smp_rmb();
+ count = tx_count(ACCESS_ONCE(priv->tx_head), tx_tail);
+ if (count == 0)
goto out;
- while ((tx_tail != priv->tx_head) || (priv->tx_count == TX_DESC_NUM)) {
- desc = &priv->tx_desc[priv->tx_tail];
+ while (count) {
+ desc = &priv->tx_desc[tx_tail];
if (desc->send_addr != 0) {
if (force)
desc->send_addr = 0;
@@ -381,57 +392,37 @@ static void hip04_tx_reclaim(struct net_device *ndev, bool force)
dev_kfree_skb(priv->tx_skb[tx_tail]);
priv->tx_skb[tx_tail] = NULL;
tx_tail = TX_NEXT(tx_tail);
- priv->tx_count--;
-
- if (priv->tx_count <= 0)
- break;
+ count--;
}
priv->tx_tail = tx_tail;
+ smp_wmb(); /* Ensure tx_tail visible to xmit */
- /* Ensure tx_tail & tx_count visible to xmit */
- smp_mb();
out:
-
if (pkts_compl || bytes_compl)
netdev_completed_queue(ndev, pkts_compl, bytes_compl);
- if (unlikely(netif_queue_stopped(ndev)) &&
- (priv->tx_count < TX_DESC_NUM))
+ if (unlikely(netif_queue_stopped(ndev)) && (count < (TX_DESC_NUM - 1)))
netif_wake_queue(ndev);
-}
-static void hip04_tx_clean_monitor(struct work_struct *work)
-{
- struct hip04_priv *priv = container_of(work, struct hip04_priv,
- tx_clean_task.work);
- struct net_device *ndev = priv->ndev;
- int delta_in_ticks = msecs_to_jiffies(1000);
-
- if (!time_in_range(jiffies, priv->last_tx,
- priv->last_tx + delta_in_ticks)) {
- netif_tx_lock(ndev);
- hip04_tx_reclaim(ndev, false);
- netif_tx_unlock(ndev);
- }
- queue_delayed_work(priv->wq, &priv->tx_clean_task, delta_in_ticks);
+ return count;
}
static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
{
struct hip04_priv *priv = netdev_priv(ndev);
struct net_device_stats *stats = &ndev->stats;
- unsigned int tx_head = priv->tx_head;
+ unsigned int tx_head = priv->tx_head, count;
struct tx_desc *desc = &priv->tx_desc[tx_head];
dma_addr_t phys;
- if (priv->tx_count >= TX_DESC_NUM) {
+ smp_rmb();
+ count = tx_count(tx_head, ACCESS_ONCE(priv->tx_tail));
+ if (count == (TX_DESC_NUM - 1)) {
netif_stop_queue(ndev);
return NETDEV_TX_BUSY;
}
- hip04_tx_reclaim(ndev, false);
-
phys = dma_map_single(&ndev->dev, skb->data, skb->len, DMA_TO_DEVICE);
if (dma_mapping_error(&ndev->dev, phys)) {
dev_kfree_skb(skb);
@@ -447,20 +438,33 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
desc->wb_addr = cpu_to_be32(phys);
skb_tx_timestamp(skb);
- /* Don't wait up for transmitted skbs to be freed. */
- skb_orphan(skb);
-
+ /* FIXME: eliminate this mmio access if xmit_more is set */
hip04_set_xmit_desc(priv, phys);
priv->tx_head = TX_NEXT(tx_head);
+ count++;
netdev_sent_queue(ndev, skb->len);
stats->tx_bytes += skb->len;
stats->tx_packets++;
- priv->tx_count++;
- priv->last_tx = jiffies;
- /* Ensure tx_head & tx_count update visible to tx reclaim */
- smp_mb();
+ /* Ensure tx_head update visible to tx reclaim */
+ smp_wmb();
+
+ /* queue is getting full, better start cleaning up now */
+ if (count >= priv->tx_coalesce_frames) {
+ if (napi_schedule_prep(&priv->napi)) {
+ /* disable rx interrupt and timer */
+ priv->reg_inten &= ~(RCV_INT);
+ writel_relaxed(DEF_INT_MASK & ~RCV_INT,
+ priv->base + PPE_INTEN);
+ hrtimer_cancel(&priv->tx_coalesce_timer);
+ __napi_schedule(&priv->napi);
+ }
+ } else if (!hrtimer_is_queued(&priv->tx_coalesce_timer)) {
+ /* cleanup not pending yet, start a new timer */
+ hrtimer_start_expires(&priv->tx_coalesce_timer,
+ HRTIMER_MODE_REL);
+ }
return NETDEV_TX_OK;
}
@@ -477,6 +481,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
bool last = false;
dma_addr_t phys;
int rx = 0;
+ int tx_remaining;
u16 len;
u32 err;
@@ -513,11 +518,11 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
buf = netdev_alloc_frag(priv->rx_buf_size);
if (!buf)
- return -ENOMEM;
+ goto done;
phys = dma_map_single(&ndev->dev, buf,
RX_BUF_SIZE, DMA_FROM_DEVICE);
if (dma_mapping_error(&ndev->dev, phys))
- return -EIO;
+ goto done;
priv->rx_buf[priv->rx_head] = buf;
priv->rx_phys[priv->rx_head] = phys;
hip04_set_recv_desc(priv, phys);
@@ -537,6 +542,11 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
}
napi_complete(napi);
done:
+ /* clean up tx descriptors and start a new timer if necessary */
+ tx_remaining = hip04_tx_reclaim(ndev, false);
+ if (rx < budget && tx_remaining)
+ hrtimer_start_expires(&priv->tx_coalesce_timer, HRTIMER_MODE_REL);
+
return rx;
}
@@ -547,6 +557,9 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
struct net_device_stats *stats = &ndev->stats;
u32 ists = readl_relaxed(priv->base + PPE_INTSTS);
+ if (!ists)
+ return IRQ_NONE;
+
writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
if (unlikely(ists & DEF_INT_ERR)) {
@@ -560,16 +573,32 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
}
}
- if (ists & RCV_INT) {
+ if (ists & RCV_INT && napi_schedule_prep(&priv->napi)) {
/* disable rx interrupt */
priv->reg_inten &= ~(RCV_INT);
- writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
- napi_schedule(&priv->napi);
+ writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
+ hrtimer_cancel(&priv->tx_coalesce_timer);
+ __napi_schedule(&priv->napi);
}
return IRQ_HANDLED;
}
+enum hrtimer_restart tx_done(struct hrtimer *hrtimer)
+{
+ struct hip04_priv *priv;
+ priv = container_of(hrtimer, struct hip04_priv, tx_coalesce_timer);
+
+ if (napi_schedule_prep(&priv->napi)) {
+ /* disable rx interrupt */
+ priv->reg_inten &= ~(RCV_INT);
+ writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
+ __napi_schedule(&priv->napi);
+ }
+
+ return HRTIMER_NORESTART;
+}
+
static void hip04_adjust_link(struct net_device *ndev)
{
struct hip04_priv *priv = netdev_priv(ndev);
@@ -589,7 +618,6 @@ static int hip04_mac_open(struct net_device *ndev)
priv->rx_head = 0;
priv->tx_head = 0;
priv->tx_tail = 0;
- priv->tx_count = 0;
hip04_reset_ppe(priv);
for (i = 0; i < RX_DESC_NUM; i++) {
@@ -612,9 +640,6 @@ static int hip04_mac_open(struct net_device *ndev)
hip04_mac_enable(ndev);
napi_enable(&priv->napi);
- INIT_DELAYED_WORK(&priv->tx_clean_task, hip04_tx_clean_monitor);
- queue_delayed_work(priv->wq, &priv->tx_clean_task, 0);
-
return 0;
}
@@ -623,8 +648,6 @@ static int hip04_mac_stop(struct net_device *ndev)
struct hip04_priv *priv = netdev_priv(ndev);
int i;
- cancel_delayed_work_sync(&priv->tx_clean_task);
-
napi_disable(&priv->napi);
netif_stop_queue(ndev);
hip04_mac_disable(ndev);
@@ -725,6 +748,7 @@ static int hip04_mac_probe(struct platform_device *pdev)
struct hip04_priv *priv;
struct resource *res;
unsigned int irq;
+ ktime_t txtime;
int ret;
ndev = alloc_etherdev(sizeof(struct hip04_priv));
@@ -751,6 +775,21 @@ static int hip04_mac_probe(struct platform_device *pdev)
priv->port = arg.args[0];
priv->chan = arg.args[1] * RX_DESC_NUM;
+ hrtimer_init(&priv->tx_coalesce_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+
+ /*
+ * BQL will try to keep the TX queue as short as possible, but it can't
+ * be faster than tx_coalesce_usecs, so we need a fast timeout here,
+ * but also long enough to gather up enough frames to ensure we don't
+ * get more interrupts than necessary.
+ * 200us is enough for 16 frames of 1500 bytes at gigabit ethernet rate
+ */
+ priv->tx_coalesce_frames = TX_DESC_NUM * 3 / 4;
+ priv->tx_coalesce_usecs = 200;
+ /* allow timer to fire after half the time at the earliest */
+ txtime = ktime_set(0, priv->tx_coalesce_usecs * NSEC_PER_USEC / 2);
+ hrtimer_set_expires_range(&priv->tx_coalesce_timer, txtime, txtime);
+
priv->map = syscon_node_to_regmap(arg.np);
if (IS_ERR(priv->map)) {
dev_warn(d, "no syscon hisilicon,hip04-ppe\n");
@@ -788,12 +827,6 @@ static int hip04_mac_probe(struct platform_device *pdev)
}
}
- priv->wq = create_singlethread_workqueue(ndev->name);
- if (!priv->wq) {
- ret = -ENOMEM;
- goto init_fail;
- }
-
INIT_WORK(&priv->tx_timeout_task, hip04_tx_timeout_task);
ether_setup(ndev);
@@ -848,8 +881,6 @@ static int hip04_remove(struct platform_device *pdev)
free_irq(ndev->irq, ndev);
of_node_put(priv->phy_node);
cancel_work_sync(&priv->tx_timeout_task);
- if (priv->wq)
- destroy_workqueue(priv->wq);
free_netdev(ndev);
return 0;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
