Skip to contentSkip to conversation

staging: octeon: Use alloc_ordered_workqueue

PATCH In review Bhaktipriya Shridhar 3 participants 7 messages Feb 19, 2016 – Feb 20, 2016 1 message not in the archive Staging

lore
Share & tools
lore

Bhaktipriya Shridhar

author
#0 lore raw

PATCH 1/1 staging: octeon: Use alloc_ordered_workqueue

Replaced create_singlethread_workqueue() with alloc_ordered_workqueue() with WQ_MEM_RECLAIM set, so it creates a workqueue which executes work items one at a time in the queueing order and is guaranteed to make forward progress under memory pressure.

Signed-off-by Bhaktipriya Shridhar <[email protected]>
---
 drivers/staging/octeon/ethernet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/octeon/ethernet.c +1 -1
index 8d239e2..6ee7fce 100644
@@ -677,7 +677,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	cvm_oct_poll_queue = create_singlethread_workqueue("octeon-ethernet");
+	cvm_oct_poll_queue = alloc_ordered_workqueue("octeon-ethernet",WQ_MEM_RECLAIM);
 	if (!cvm_oct_poll_queue) {
 		pr_err("octeon-ethernet: Cannot create workqueue");
 		return -ENOMEM;

--
2.1.4

#1 lore raw

Hello, Bhaktipriya.

Quoted 15 lines · Bhaktipriya Shridhar

Replaced create_singlethread_workqueue() with alloc_ordered_workqueue() with WQ_MEM_RECLAIM set, so it creates a workqueue which executes work items one at a time in the queueing order and is guaranteed to make forward progress under memory pressure.

Signed-off-by Bhaktipriya Shridhar <[email protected]>
---
 drivers/staging/octeon/ethernet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/octeon/ethernet.c +0 -0
index 8d239e2..6ee7fce 100644

drivers/staging/octeon/ethernet.c static int cvm_oct_probe(struct platform_device *pdev) · line 680

Quoted 5 lines
drivers/staging/octeon/ethernet.c +1 -1
@@ -677,7 +677,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	cvm_oct_poll_queue = create_singlethread_workqueue("octeon-ethernet");
+	cvm_oct_poll_queue = alloc_ordered_workqueue("octeon-ethernet",WQ_MEM_RECLAIM);

So, this is the mechanical conversion from create_singlethread_workqueue() to the new interface; however, that's not what we want to do. Does it *need* to be ordered? Does it *need* to be guaranteed to make forward progress because it's depended upon during memory reclaim? Does it even *need* to be a separate workqueue? You need to actually examine the specific use case and answer those questions before converting it and explain and justify the decision in the changelog.

Thanks.

Signature · 1 line
tejun
author
#2 lore raw

Hello Tejun,

Thank you for your reply.

Quoted 9 lines · Tejun Heo

So, this is the mechanical conversion from create_singlethread_workqueue() to the new interface; however, that's not what we want to do. Does it *need* to be ordered? Does it *need* to be guaranteed to make forward progress because it's depended upon during memory reclaim? Does it even *need* to be a separate workqueue? You need to actually examine the specific use case and answer those questions before converting it and explain and justify the decision in the changelog.

According to my understanding, with cmwq, there's no need for octeon/ethernet driver to use a separate workqueue. I have removed the dedicated workqueue from ethernet and have used system_wq instead.
Please correct me I am wrong.

Thanks,
Bhaktipriya

A message that is not in the archive (sent off-list or not yet received): CAFNHFT96V4Pt==M0ZRup-iZDn3FE1SZ8T_m_sxqsg6gqQ=7K1Q@mail.gmail.com

#4 lore raw

Hello, Bhakti.

Quoted 10 lines · Bhakti Priya
Quoted 9 lines · Tejun Heo

So, this is the mechanical conversion from create_singlethread_workqueue() to the new interface; however, that's not what we want to do. Does it *need* to be ordered? Does it *need* to be guaranteed to make forward progress because it's depended upon during memory reclaim? Does it even *need* to be a separate workqueue? You need to actually examine the specific use case and answer those questions before converting it and explain and justify the decision in the changelog.

Quoted 5 lines

According to my understanding, with cmwq, there's no need for octeon/ethernet driver to use separate
a workqueue. I have removed the dedicated workqueues from ethernet and have used
system_wq instead. Please correct me I am wrong.

I think that'd be the case but can you please explain how you reached that conclusion? Explaining the process or justifications is often as or even more important as reaching the right conclusion as that allows other people to verify its correctness and helps correcting it if it turns out to be wrong.

Thanks!

Signature · 1 line
tejun
author
#5 lore raw

Hello Tejun,

I absolutely agree with you. And here's what I've understood.

Quoted 17 lines · Tejun Heo
Quoted 10 lines · Bhakti Priya
Quoted 9 lines · Tejun Heo

So, this is the mechanical conversion from create_singlethread_workqueue() to the new interface; however, that's not what we want to do. Does it *need* to be ordered? Does it *need* to be guaranteed to make forward progress because it's depended upon during memory reclaim? Does it even *need* to be a separate workqueue? You need to actually examine the specific use case and answer those questions before converting it and explain and justify the decision in the changelog.

Quoted 5 lines

According to my understanding, with cmwq, there's no need for octeon/ethernet driver to use separate
a workqueue. I have removed the dedicated workqueues from ethernet and have used
system_wq instead. Please correct me I am wrong.

Quoted 6 lines

I think that'd be the case but can you please explain how you reached that conclusion? Explaining the process or justifications is often as or even more important as reaching the right conclusion as that allows other people to verify its correctness and helps correcting it if it turns out to be wrong.

In most cases, creating a whole set of worker threads in order to run a function is overkill. Also, there is no difference in execution characteristics between using a dedicated wq and a system wq. The workqueue in octeon/ethernet is a singlethread_workqueue and is not cpu-intensive or high-priority. Hence system_wq is preferred.

Thanks,
Bhaktipriya

#6 lore raw
Quoted 29 lines · Bhakti Priya

Hello Tejun,

I absolutely agree with you. And here's what I've understood.

Quoted 17 lines · Tejun Heo
Quoted 10 lines · Bhakti Priya
Quoted 9 lines · Tejun Heo

So, this is the mechanical conversion from create_singlethread_workqueue() to the new interface; however, that's not what we want to do. Does it *need* to be ordered? Does it *need* to be guaranteed to make forward progress because it's depended upon during memory reclaim? Does it even *need* to be a separate workqueue? You need to actually examine the specific use case and answer those questions before converting it and explain and justify the decision in the changelog.

Quoted 5 lines

According to my understanding, with cmwq, there's no need for octeon/ethernet driver to use separate
a workqueue. I have removed the dedicated workqueues from ethernet and have used
system_wq instead. Please correct me I am wrong.

Quoted 6 lines

I think that'd be the case but can you please explain how you reached that conclusion? Explaining the process or justifications is often as or even more important as reaching the right conclusion as that allows other people to verify its correctness and helps correcting it if it turns out to be wrong.

Quoted 5 lines

In most cases, creating a whole set of worker threads in order to run a function is overkill. Also, there is no difference in execution characteristics between using a dedicated wq and a system wq. The workqueue in octeon/ethernet is a singlethread_workqueue and is not cpu-intensive or high-priority. Hence system_wq is preferred.

The explanation should be in the patch, so that people who look at the commit logs in the future can underatand the reasoning involved.

Not knowing much about this problem, I'm not sure to understand:

Also, there is no difference in execution characteristics between using a dedicated wq and a system wq.

Is this a general statement, or is ti particular to this case?

thanks,
julia

author
#7 lore raw

Hello Julia,

Thank you for you reply.

Quoted 6 lines · Julia Lawall

The explanation should be in the patch, so that people who look at the commit logs in the future can underatand the reasoning involved.

Like you suggested, I have mentioned my explanation and reasoning in
v2 of the patch.

Quoted 7 lines

Not knowing much about this problem, I'm not sure to understand:

Also, there is no difference in execution characteristics between using a dedicated wq and a system wq.

Is this a general statement, or is ti particular to this case?

It is a general statement for system_wq. Hence, I have removed this statement from my patch.

Thanks,
Bhaktipriya