Skip to contentSkip to conversation

staging: octeon: Use system_wq instead of dedicated workqueue

PATCH In review Bhaktipriya Shridhar 2 participants 3 messages Feb 20, 2016 – Feb 25, 2016 Staging

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 staging: octeon: Use system_wq instead of dedicated workqueue

With cmwq, there's no need for octeon/ethernet driver to use a separate workqueue. Removed the dedicated workqueue from ethernet and used system_wq instead.

Here, creating a whole set of worker threads in order to run a function is an overkill. The workqueue in octeon/ethernet is a singlethread_workqueue and is not cpu-intensive or high-priority. Hence system_wq is preferred.

---
 drivers/staging/octeon/ethernet.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)
drivers/staging/octeon/ethernet.c +4 -17
index 8d239e2..c193f88 100644
@@ -86,10 +86,6 @@ int rx_napi_weight = 32;
 module_param(rx_napi_weight, int, 0444);
 MODULE_PARM_DESC(rx_napi_weight, "The NAPI WEIGHT parameter.");
 
-/*
- * cvm_oct_poll_queue - Workqueue for polling operations.
- */
-struct workqueue_struct *cvm_oct_poll_queue;
 
 /*
  * cvm_oct_poll_queue_stopping - flag to indicate polling should stop.
@@ -121,8 +117,7 @@ static void cvm_oct_rx_refill_worker(struct work_struct *work)
 	cvm_oct_rx_refill_pool(num_packet_buffers / 2);
 
 	if (!atomic_read(&cvm_oct_poll_queue_stopping))
-		queue_delayed_work(cvm_oct_poll_queue,
-				   &cvm_oct_rx_refill_work, HZ);
+		schedule_delayed_work(&cvm_oct_rx_refill_work, HZ);
 }
 
 static void cvm_oct_periodic_worker(struct work_struct *work)
@@ -138,8 +133,7 @@ static void cvm_oct_periodic_worker(struct work_struct *work)
 						cvm_oct_device[priv->port]);
 
 	if (!atomic_read(&cvm_oct_poll_queue_stopping))
-		queue_delayed_work(cvm_oct_poll_queue,
-						&priv->port_periodic_work, HZ);
+		schedule_delayed_work(&priv->port_periodic_work, HZ);
 }
 
 static void cvm_oct_configure_common_hw(void)
@@ -677,11 +671,6 @@ static int cvm_oct_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	cvm_oct_poll_queue = create_singlethread_workqueue("octeon-ethernet");
-	if (!cvm_oct_poll_queue) {
-		pr_err("octeon-ethernet: Cannot create workqueue");
-		return -ENOMEM;
-	}
 
 	cvm_oct_configure_common_hw();
 
@@ -840,8 +829,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
 				fau -=
 				    cvmx_pko_get_num_queues(priv->port) *
 				    sizeof(u32);
-				queue_delayed_work(cvm_oct_poll_queue,
-						&priv->port_periodic_work, HZ);
+				schedule_delayed_work(&priv->port_periodic_work, HZ);
 			}
 		}
 	}
@@ -854,7 +842,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
 	 */
 	cvm_oct_tx_poll_interval = 150 * (octeon_get_clock_rate() / 1000000);
 
-	queue_delayed_work(cvm_oct_poll_queue, &cvm_oct_rx_refill_work, HZ);
+	schedule_delayed_work(&cvm_oct_rx_refill_work, HZ);
 
 	return 0;
 }
@@ -897,7 +885,6 @@ static int cvm_oct_remove(struct platform_device *pdev)
 		}
 	}
 
-	destroy_workqueue(cvm_oct_poll_queue);
 
 	cvmx_pko_shutdown();
 

--
2.1.4

#1 lore raw

Hello Tejun,

I have removed dedicated workqueue usages from octeon/ethernet driver. Can you please review this patch.

Thanks,
Bhaktipriya

Quoted 90 lines · Bhaktipriya Shridhar

With cmwq, there's no need for octeon/ethernet driver to use a separate workqueue. Removed the dedicated workqueue from ethernet and used system_wq instead.

Here, creating a whole set of worker threads in order to run a function is an overkill. The workqueue in octeon/ethernet is a singlethread_workqueue and is not cpu-intensive or high-priority. Hence system_wq is preferred.

---
 drivers/staging/octeon/ethernet.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)
drivers/staging/octeon/ethernet.c +4 -18
index 8d239e2..c193f88 100644
@@ -86,10 +86,6 @@ int rx_napi_weight = 32;
 module_param(rx_napi_weight, int, 0444);
 MODULE_PARM_DESC(rx_napi_weight, "The NAPI WEIGHT parameter.");
 
-/*
- * cvm_oct_poll_queue - Workqueue for polling operations.
- */
-struct workqueue_struct *cvm_oct_poll_queue;
 
 /*
  * cvm_oct_poll_queue_stopping - flag to indicate polling should stop.
@@ -121,8 +117,7 @@ static void cvm_oct_rx_refill_worker(struct work_struct *work)
        cvm_oct_rx_refill_pool(num_packet_buffers / 2);
 
        if (!atomic_read(&cvm_oct_poll_queue_stopping))
-               queue_delayed_work(cvm_oct_poll_queue,
-                                  &cvm_oct_rx_refill_work, HZ);
+               schedule_delayed_work(&cvm_oct_rx_refill_work, HZ);
 }
 
 static void cvm_oct_periodic_worker(struct work_struct *work)
@@ -138,8 +133,7 @@ static void cvm_oct_periodic_worker(struct work_struct *work)
                                                cvm_oct_device[priv->port]);
 
        if (!atomic_read(&cvm_oct_poll_queue_stopping))
-               queue_delayed_work(cvm_oct_poll_queue,
-                                               &priv->port_periodic_work, HZ);
+               schedule_delayed_work(&priv->port_periodic_work, HZ);
 }
 
 static void cvm_oct_configure_common_hw(void)
@@ -677,11 +671,6 @@ static int cvm_oct_probe(struct platform_device *pdev)
                return -EINVAL;
        }
 
-       cvm_oct_poll_queue = create_singlethread_workqueue("octeon-ethernet");
-       if (!cvm_oct_poll_queue) {
-               pr_err("octeon-ethernet: Cannot create workqueue");
-               return -ENOMEM;
-       }
 
        cvm_oct_configure_common_hw();
 
@@ -840,8 +829,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
                                fau -=
                                    cvmx_pko_get_num_queues(priv->port) *
                                    sizeof(u32);
-                               queue_delayed_work(cvm_oct_poll_queue,
-                                               &priv->port_periodic_work, HZ);
+                               schedule_delayed_work(&priv->port_periodic_work, HZ);
                        }
                }
        }
@@ -854,7 +842,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
         */
        cvm_oct_tx_poll_interval = 150 * (octeon_get_clock_rate() / 1000000);
 
-       queue_delayed_work(cvm_oct_poll_queue, &cvm_oct_rx_refill_work, HZ);
+       schedule_delayed_work(&cvm_oct_rx_refill_work, HZ);
 
        return 0;
 }
@@ -897,7 +885,6 @@ static int cvm_oct_remove(struct platform_device *pdev)
                }
        }
 
-       destroy_workqueue(cvm_oct_poll_queue);
 
        cvmx_pko_shutdown();
 
--

2.1.4

#2 lore raw
In reply to Bhaktipriya Shridhar

Hello, Bhaktipriya.

Sorry about the delay. Was buried past several days.

Quoted 7 lines · Bhaktipriya Shridhar

With cmwq, there's no need for octeon/ethernet driver to use a separate workqueue. Removed the dedicated workqueue from ethernet and used system_wq instead.

Here, creating a whole set of worker threads in order to run a function is an overkill. The workqueue in octeon/ethernet is a singlethread_workqueue and is not cpu-intensive or high-priority. Hence system_wq is preferred.

So, while the patch looks good to me, I think the description still needs to be more detailed.

* Why don't we need WQ_RESCUER?

* What about ordering requirements?

* Do the work items need flushing? If so, how are they handled? If not, why?

Thanks.

Signature · 1 line
tejun