[PATCH net-next 0/2] Improvements to the DSA deferred xmit

[PATCH net-next 0/2] Improvements to the DSA deferred xmit

[Vladimir Oltean]

The DSA deferred xmit mechanism is currently used by a single driver
(sja1105) because the transmission of some operations requires SPI
access in the fastpath.

This 2-patch series makes this mechanism better for everybody:

- For those who don't use it, thanks to one less assignment in the
  hotpath
- For those who do, by making its scheduling more amenable and moving it
  outside the generic workqueue (since it still deals with packet
  hotpath, after all)

Vladimir Oltean (2):
  net: dsa: Remove deferred_xmit from dsa_skb_cb
  net: dsa: Create a kernel thread for each port's deferred xmit work

 include/net/dsa.h |  4 ++--
 net/dsa/slave.c   | 53 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 41 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH net-next 1/2] net: dsa: Remove deferred_xmit from dsa_skb_cb

[Vladimir Oltean]

The introduction of the deferred xmit mechanism in DSA has made the
hotpath slightly more inefficient for everybody, since
DSA_SKB_CB(skb)->deferred_xmit needed to be initialized to false for
every transmitted frame, in order to figure out whether the driver
requested deferral or not. That was necessary to avoid kfree_skb from
freeing the skb.

But actually we can just remove that variable in the skb->cb and
counteract the effect of kfree_skb with skb_get, much to the same
effect. The advantage, of course, being that anybody who doesn't use
deferred xmit doesn't need to do any extra operation in the hotpath.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 include/net/dsa.h |  1 -
 net/dsa/slave.c   | 12 ++++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 6767dc3f66c0..5d510a4da5d0 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -88,7 +88,6 @@ struct dsa_device_ops {
 
 struct dsa_skb_cb {
 	struct sk_buff *clone;
-	bool deferred_xmit;
 };
 
 struct __dsa_skb_cb {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 78ffc87dc25e..9f7e47dcdc20 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -518,7 +518,6 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 	s->tx_bytes += skb->len;
 	u64_stats_update_end(&s->syncp);
 
-	DSA_SKB_CB(skb)->deferred_xmit = false;
 	DSA_SKB_CB(skb)->clone = NULL;
 
 	/* Identify PTP protocol packets, clone them, and pass them to the
@@ -531,8 +530,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 	 */
 	nskb = p->xmit(skb, dev);
 	if (!nskb) {
-		if (!DSA_SKB_CB(skb)->deferred_xmit)
-			kfree_skb(skb);
+		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}
 
@@ -543,10 +541,12 @@ void *dsa_defer_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 
-	DSA_SKB_CB(skb)->deferred_xmit = true;
-
-	skb_queue_tail(&dp->xmit_queue, skb);
+	/* Increase refcount so the kfree_skb in dsa_slave_xmit
+	 * won't really free the packet.
+	 */
+	skb_queue_tail(&dp->xmit_queue, skb_get(skb));
 	schedule_work(&dp->xmit_work);
+
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(dsa_defer_xmit);
-- 
2.17.1

[PATCH net-next 2/2] net: dsa: Create a kernel thread for each port's deferred xmit work

[Vladimir Oltean]

Because this callback can be used on certain switches (at the moment
only sja1105) to do TX timstamping, this should have been a kernel
thread from the get-go.

PTP is a time-critical protocol and requires the ability to control the
scheduling priority with tools such as chrt, as opposed to just
executing the work in the generic workqueue.

If the user switch ports are named swp0, swp1, swp2, the kernel threads
will be named dsa_swp0_xmit, dsa_swp1_xmit, dsa_swp2_xmit.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 include/net/dsa.h |  3 ++-
 net/dsa/slave.c   | 43 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 5d510a4da5d0..8460eae69dd9 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -189,7 +189,8 @@ struct dsa_port {
 	struct phylink		*pl;
 	struct phylink_config	pl_config;
 
-	struct work_struct	xmit_work;
+	struct kthread_worker	*xmit_worker;
+	struct kthread_work	xmit_work;
 	struct sk_buff_head	xmit_queue;
 
 	struct list_head list;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9f7e47dcdc20..c7e8a29e1e86 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -115,9 +115,12 @@ static int dsa_slave_close(struct net_device *dev)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
 	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = dp->ds;
 
-	cancel_work_sync(&dp->xmit_work);
-	skb_queue_purge(&dp->xmit_queue);
+	if (ds->ops->port_deferred_xmit) {
+		kthread_cancel_work_sync(&dp->xmit_work);
+		skb_queue_purge(&dp->xmit_queue);
+	}
 
 	phylink_stop(dp->pl);
 
@@ -545,13 +548,13 @@ void *dsa_defer_xmit(struct sk_buff *skb, struct net_device *dev)
 	 * won't really free the packet.
 	 */
 	skb_queue_tail(&dp->xmit_queue, skb_get(skb));
-	schedule_work(&dp->xmit_work);
+	kthread_queue_work(dp->xmit_worker, &dp->xmit_work);
 
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(dsa_defer_xmit);
 
-static void dsa_port_xmit_work(struct work_struct *work)
+static void dsa_port_xmit_work(struct kthread_work *work)
 {
 	struct dsa_port *dp = container_of(work, struct dsa_port, xmit_work);
 	struct dsa_switch *ds = dp->ds;
@@ -1363,12 +1366,15 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 int dsa_slave_suspend(struct net_device *slave_dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(slave_dev);
+	struct dsa_switch *ds = dp->ds;
 
 	if (!netif_running(slave_dev))
 		return 0;
 
-	cancel_work_sync(&dp->xmit_work);
-	skb_queue_purge(&dp->xmit_queue);
+	if (ds->ops->port_deferred_xmit) {
+		kthread_cancel_work_sync(&dp->xmit_work);
+		skb_queue_purge(&dp->xmit_queue);
+	}
 
 	netif_device_detach(slave_dev);
 
@@ -1455,17 +1461,29 @@ int dsa_slave_create(struct dsa_port *port)
 	}
 	p->dp = port;
 	INIT_LIST_HEAD(&p->mall_tc_list);
-	INIT_WORK(&port->xmit_work, dsa_port_xmit_work);
-	skb_queue_head_init(&port->xmit_queue);
 	p->xmit = cpu_dp->tag_ops->xmit;
 	port->slave = slave_dev;
 
+	if (ds->ops->port_deferred_xmit) {
+		kthread_init_work(&port->xmit_work, dsa_port_xmit_work);
+		port->xmit_worker = kthread_create_worker(0, "dsa_%s_xmit",
+							  slave_dev->name);
+		if (IS_ERR(port->xmit_worker)) {
+			ret = PTR_ERR(port->xmit_worker);
+			netdev_err(master,
+				   "failed to create deferred xmit thread: %d\n",
+				   ret);
+			goto out_free;
+		}
+		skb_queue_head_init(&port->xmit_queue);
+	}
+
 	netif_carrier_off(slave_dev);
 
 	ret = dsa_slave_phy_setup(slave_dev);
 	if (ret) {
 		netdev_err(master, "error %d setting up slave phy\n", ret);
-		goto out_free;
+		goto out_destroy;
 	}
 
 	dsa_slave_notify(slave_dev, DSA_PORT_REGISTER);
@@ -1484,6 +1502,9 @@ int dsa_slave_create(struct dsa_port *port)
 	phylink_disconnect_phy(p->dp->pl);
 	rtnl_unlock();
 	phylink_destroy(p->dp->pl);
+out_destroy:
+	if (ds->ops->port_deferred_xmit)
+		kthread_destroy_worker(port->xmit_worker);
 out_free:
 	free_percpu(p->stats64);
 	free_netdev(slave_dev);
@@ -1495,6 +1516,10 @@ void dsa_slave_destroy(struct net_device *slave_dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(slave_dev);
 	struct dsa_slave_priv *p = netdev_priv(slave_dev);
+	struct dsa_switch *ds = dp->ds;
+
+	if (ds->ops->port_deferred_xmit)
+		kthread_destroy_worker(dp->xmit_worker);
 
 	netif_carrier_off(slave_dev);
 	rtnl_lock();
-- 
2.17.1

Re: [PATCH net-next 0/2] Improvements to the DSA deferred xmit

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Fri, 27 Dec 2019 03:42:06 +0200

> The DSA deferred xmit mechanism is currently used by a single driver
> (sja1105) because the transmission of some operations requires SPI
> access in the fastpath.
> 
> This 2-patch series makes this mechanism better for everybody:
> 
> - For those who don't use it, thanks to one less assignment in the
>   hotpath
> - For those who do, by making its scheduling more amenable and moving it
>   outside the generic workqueue (since it still deals with packet
>   hotpath, after all)

Two comments about this patch series, I think it needs more work:

1) This adds the thread and the xmit queue but not code that actually
   uses it.  You really have to provide the support code in the driver
   at the same time you add the new facitlity so we can actually see
   how it'll be used.

2) Patch #1 talks about a tradeoff.  Replacing the CB initialization of
   the field skb_get().  But this skb_get() is an atomic operation and
   thus much more expensive for users of the deferred xmit scheme.

Thanks.

Re: [PATCH net-next 0/2] Improvements to the DSA deferred xmit

[Vladimir Oltean]

Hi David,

On Thu, 2 Jan 2020 at 23:49, David Miller <davem@davemloft.net> wrote:
>
> Two comments about this patch series, I think it needs more work:
>

Thanks for looking at this series.

> 1) This adds the thread and the xmit queue but not code that actually
>    uses it.  You really have to provide the support code in the driver
>    at the same time you add the new facitlity so we can actually see
>    how it'll be used.
>

There is no API change here. There was, and still is, a single caller
of dsa_defer_xmit in the kernel, from net/dsa/tag_sja1105.c:

    if (unlikely(sja1105_is_link_local(skb)))
        return dsa_defer_xmit(skb, netdev);

The whole difference is that what used to be a schedule_work() in that
function is now a kthread_queue_work() call.

> 2) Patch #1 talks about a tradeoff.  Replacing the CB initialization of
>    the field skb_get().  But this skb_get() is an atomic operation and
>    thus much more expensive for users of the deferred xmit scheme.
>

Ok, I'll admit I hadn't considered the exact penalty introduced by
skb_get, but I think it is a matter of proportions.
Worst case I expect no more than 64 packets per second to be
transmitted using the deferred xmit mechanism: there are 4 switch
ports, PTP runs with a sync interval of 1/8 seconds, and the STP hello
timer is 2 seconds. So, not a lot of traffic.
On the other hand, clearing the deferred_xmit bool from the skb->cb is
something that everybody else (including this driver for "normal"
traffic) needs to do at line rate, just for the above 64 packets per
second (in the worst case) to be possible.

> Thanks.

Regards,
-Vladimir

Re: [PATCH net-next 0/2] Improvements to the DSA deferred xmit

[Florian Fainelli]

On 1/2/20 2:47 PM, Vladimir Oltean wrote:
> Hi David,
> 
> On Thu, 2 Jan 2020 at 23:49, David Miller <davem@davemloft.net> wrote:
>>
>> Two comments about this patch series, I think it needs more work:
>>
> 
> Thanks for looking at this series.
> 
>> 1) This adds the thread and the xmit queue but not code that actually
>>    uses it.  You really have to provide the support code in the driver
>>    at the same time you add the new facitlity so we can actually see
>>    how it'll be used.
>>
> 
> There is no API change here. There was, and still is, a single caller
> of dsa_defer_xmit in the kernel, from net/dsa/tag_sja1105.c:
> 
>     if (unlikely(sja1105_is_link_local(skb)))
>         return dsa_defer_xmit(skb, netdev);
> 
> The whole difference is that what used to be a schedule_work() in that
> function is now a kthread_queue_work() call.
> 
>> 2) Patch #1 talks about a tradeoff.  Replacing the CB initialization of
>>    the field skb_get().  But this skb_get() is an atomic operation and
>>    thus much more expensive for users of the deferred xmit scheme.
>>
> 
> Ok, I'll admit I hadn't considered the exact penalty introduced by
> skb_get, but I think it is a matter of proportions.
> Worst case I expect no more than 64 packets per second to be
> transmitted using the deferred xmit mechanism: there are 4 switch
> ports, PTP runs with a sync interval of 1/8 seconds, and the STP hello
> timer is 2 seconds. So, not a lot of traffic.
> On the other hand, clearing the deferred_xmit bool from the skb->cb is
> something that everybody else (including this driver for "normal"
> traffic) needs to do at line rate, just for the above 64 packets per
> second (in the worst case) to be possible.

I sincerely think your transmit path is so radically different that your
sja1105 driver should simply be absorbing all of this logic and the core
DSA framework should be free of any deferred transmit logic. Can you
consider doing that before the merge window ends?
-- 
Florian

Re: [PATCH net-next 0/2] Improvements to the DSA deferred xmit

[Richard Cochran]

On Fri, Jan 03, 2020 at 12:10:44PM -0800, Florian Fainelli wrote:
> I sincerely think your transmit path is so radically different that your
> sja1105 driver should simply be absorbing all of this logic and the core
> DSA framework should be free of any deferred transmit logic. Can you
> consider doing that before the merge window ends?

+1

Thanks,
Richard