Skip to contentSkip to conversation

s390/oprofile: Remove deprecated create_workqueue

PATCH In review Bhaktipriya Shridhar 6 participants 9 messages Jun 7, 2016 – Jun 17, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 s390/oprofile: Remove deprecated create_workqueue

A dedicated workqueue has been used since the workqueue hws_wq with workitem &cb->worker, is involved in hardware based sampling on System z processors.

Since, these are long-running work items and aren't involved in memory reclaim in any way, system_long_wq has been used.

---
 arch/s390/oprofile/hwsampler.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)
arch/s390/oprofile/hwsampler.c +3 -18
index ff9b4eb..8b16bb8 100644
@@ -42,7 +42,6 @@ static DEFINE_MUTEX(hws_sem_oom);
 static unsigned char hws_flush_all;
 static unsigned int hws_oom;
 static unsigned int hws_alert;
-static struct workqueue_struct *hws_wq;
 
 static unsigned int hws_state;
 enum {
@@ -189,8 +188,7 @@ static void hws_ext_handler(struct ext_code ext_code,
 	inc_irq_stat(IRQEXT_CMS);
 	atomic_xchg(&cb->ext_params, atomic_read(&cb->ext_params) | param32);
 
-	if (hws_wq)
-		queue_work(hws_wq, &cb->worker);
+		queue_work(system_long_wq, &cb->worker);
 }
 
 static void worker(struct work_struct *work);
@@ -566,15 +564,12 @@ int hwsampler_deactivate(unsigned int cpu)
 			} else  {
 				hws_flush_all = 1;
 				/* Add work to queue to read pending samples.*/
-				queue_work_on(cpu, hws_wq, &cb->worker);
+				queue_work_on(cpu, system_long_wq, &cb->worker);
 			}
 		}
 	}
 	mutex_unlock(&hws_sem);
 
-	if (hws_wq)
-		flush_workqueue(hws_wq);
-
 	return rc;
 }
 
@@ -740,7 +735,7 @@ static void worker_on_finish(unsigned int cpu)
 					continue;
 				if (!cb->finish) {
 					cb->finish = 1;
-					queue_work_on(i, hws_wq,
+					queue_work_on(i, system_long_wq,
 						&cb->worker);
 				}
 			}
@@ -997,9 +992,6 @@ int hwsampler_setup(void)
 		goto setup_exit;
 
 	rc = -EINVAL;
-	hws_wq = create_workqueue("hwsampler");
-	if (!hws_wq)
-		goto setup_exit;
 
 	register_cpu_notifier(&hws_cpu_notifier);
 
@@ -1049,9 +1041,6 @@ int hwsampler_shutdown(void)
 	if (hws_state == HWS_DEALLOCATED || hws_state == HWS_STOPPED) {
 		mutex_unlock(&hws_sem);
 
-		if (hws_wq)
-			flush_workqueue(hws_wq);
-
 		mutex_lock(&hws_sem);
 
 		if (hws_state == HWS_STOPPED) {
@@ -1059,10 +1048,6 @@ int hwsampler_shutdown(void)
 			hws_alert = 0;
 			deallocate_sdbt();
 		}
-		if (hws_wq) {
-			destroy_workqueue(hws_wq);
-			hws_wq = NULL;
-		}
 
 		unregister_external_irq(EXT_IRQ_MEASURE_ALERT, hws_ext_handler);
 		hws_state = HWS_INIT;

--
2.1.4

#1 lore raw
reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree…

Hi,

[auto build test ERROR on s390/features]
[also build test ERROR on v4.7-rc2 next-20160607]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Bhaktipriya-Shridhar/s390-oprofile-Remove-deprecated-create_workqueue/20160608-060615
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-default_defconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205

reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390

All errors (new ones prefixed by >>):

arch/s390/oprofile/hwsampler.c: In function 'hwsampler_stop_all':

>> arch/s390/oprofile/hwsampler.c:1151:6: error: 'hws_wq' undeclared (first use in this function)
     if (hws_wq)
         ^

arch/s390/oprofile/hwsampler.c:1151:6: note: each undeclared identifier is reported only once for each function it appears in

vim +/hws_wq +1151 arch/s390/oprofile/hwsampler.c

ec6a3df1 Heinz Graalfs 2011-01-21  1145  		cb->stop_mode = 1;
ec6a3df1 Heinz Graalfs 2011-01-21  1146  		tmp_rc = stop_sampling(cpu);
ec6a3df1 Heinz Graalfs 2011-01-21  1147  		if (tmp_rc)
ec6a3df1 Heinz Graalfs 2011-01-21  1148  			rc = tmp_rc;
ec6a3df1 Heinz Graalfs 2011-01-21  1149  	}
ec6a3df1 Heinz Graalfs 2011-01-21  1150  
ec6a3df1 Heinz Graalfs 2011-01-21 @1151  	if (hws_wq)
ec6a3df1 Heinz Graalfs 2011-01-21  1152  		flush_workqueue(hws_wq);
ec6a3df1 Heinz Graalfs 2011-01-21  1153  
ec6a3df1 Heinz Graalfs 2011-01-21  1154  	mutex_lock(&hws_sem);

:::::: The code at line 1151 was first introduced by commit
:::::: ec6a3df1c008d9e8664e53b0363f6847c5c0dc3f oprofile, s390: Add support for hardware based sampling on System z processors

:::::: TO: Heinz Graalfs <[email protected]> :::::: CC: Robert Richter <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#2 lore raw
In reply to Bhaktipriya Shridhar
Quoted 11 lines · Bhaktipriya Shridhar

A dedicated workqueue has been used since the workqueue hws_wq with workitem &cb->worker, is involved in hardware based sampling on System z processors.

Since, these are long-running work items and aren't involved in memory reclaim in any way, system_long_wq has been used.

---
 arch/s390/oprofile/hwsampler.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

Besides that your patch doesn't compile... ;)

Removing the flush_workqueue is probably wrong, since the oprofile module can now be unloaded while there could still be a reference to a worker structure owned by the oprofile module.

However I'm wondering if we shouldn't simply remove at least the s390 specific hwswampler code from the oprofile module. This would still leave the common code timer based sampling mode for oprofile working on s390.

It looks like the oprofile user space utility nowadays (since 2012) uses the kernel perf interface instead of the oprofile interface anyway, if present. So the oprofile module itself doesn't seem to have too many users left.

Any opinions?

Quoted 21 lines
arch/s390/oprofile/hwsampler.c +0 -8
-		if (hws_wq)
-			flush_workqueue(hws_wq);
-
 		mutex_lock(&hws_sem);
 
 		if (hws_state == HWS_STOPPED) {
@@ -1059,10 +1048,6 @@ int hwsampler_shutdown(void)
 			hws_alert = 0;
 			deallocate_sdbt();
 		}
-		if (hws_wq) {
-			destroy_workqueue(hws_wq);
-			hws_wq = NULL;
-		}
 
 		unregister_external_irq(EXT_IRQ_MEASURE_ALERT, hws_ext_handler);
 		hws_state = HWS_INIT;
--

2.1.4

#3 lore raw

Heiko,

Quoted 10 lines · Heiko Carstens

However I'm wondering if we shouldn't simply remove at least the s390 specific hwswampler code from the oprofile module. This would still leave the common code timer based sampling mode for oprofile working on s390.

It looks like the oprofile user space utility nowadays (since 2012) uses the kernel perf interface instead of the oprofile interface anyway, if present. So the oprofile module itself doesn't seem to have too many users left.

Any opinions?

yes, the kernel driver is not necessary for oprofile userland for a while now. There is no ongoing development any longer, most patches are due to changes in the kernel apis.

So if there is code that needs a larger rework due to other kernel changes and there is no user anymore, I am fine with removing the code instead of reworking it. I still would just keep existing code as long as we can keep it unchanged (some like the lightwight of oprofile, esp. in the embedded space). If there is a user of the code, a Tested-by would be good for new code changes.

If there are users of the hwswampler, speak up now. Else, let's just remove it.

-Robert

#4 lore raw
Quoted 25 lines · Robert Richter

Heiko,

Quoted 10 lines · Heiko Carstens

However I'm wondering if we shouldn't simply remove at least the s390 specific hwswampler code from the oprofile module. This would still leave the common code timer based sampling mode for oprofile working on s390.

It looks like the oprofile user space utility nowadays (since 2012) uses the kernel perf interface instead of the oprofile interface anyway, if present. So the oprofile module itself doesn't seem to have too many users left.

Any opinions?

yes, the kernel driver is not necessary for oprofile userland for a while now. There is no ongoing development any longer, most patches are due to changes in the kernel apis.

So if there is code that needs a larger rework due to other kernel changes and there is no user anymore, I am fine with removing the code instead of reworking it. I still would just keep existing code as long as we can keep it unchanged (some like the lightwight of oprofile, esp. in the embedded space). If there is a user of the code, a Tested-by would be good for new code changes.

Quoted 5 lines

If there are users of the hwswampler, speak up now. Else, let's just remove it.

-Robert

Hi,

As Robert mentioned the user-space oprofile code that would have used the oprofile device driver was removed around August 2014 in preparation for oprofile-1.0. The operf command which uses the kernel perf infrastructure and does not need for the oprofile kernel driver has been in oprofile since August of 2012. For some architectures it would make sense to simplify things by eliminate the oprofile kernel driver.

-Will Cohen

#5 lore raw
In reply to Robert Richter
Quoted 25 lines · Robert Richter

Heiko,

Quoted 10 lines · Heiko Carstens

However I'm wondering if we shouldn't simply remove at least the s390 specific hwswampler code from the oprofile module. This would still leave the common code timer based sampling mode for oprofile working on s390.

It looks like the oprofile user space utility nowadays (since 2012) uses the kernel perf interface instead of the oprofile interface anyway, if present. So the oprofile module itself doesn't seem to have too many users left.

Any opinions?

yes, the kernel driver is not necessary for oprofile userland for a while now. There is no ongoing development any longer, most patches are due to changes in the kernel apis.

So if there is code that needs a larger rework due to other kernel changes and there is no user anymore, I am fine with removing the code instead of reworking it. I still would just keep existing code as long as we can keep it unchanged (some like the lightwight of oprofile, esp. in the embedded space). If there is a user of the code, a Tested-by would be good for new code changes.

Quoted 2 lines

If there are users of the hwswampler, speak up now. Else, let's just remove it.

Ok, so I'll wait a week or so and remove the code if nobody speaks up. Is it ok for you if I add the patch to the s390 kernel tree?
The patch would only remove s390 specific architecture code.

I have this pending:

s390/oprofile: remove hardware sampler support

Remove hardware sampler support from oprofile module.

The oprofile user space utilty has been switched to use the kernel perf interface, for which we also provide hardware sampling support.

In addition the hardware sampling support is also slightly broken: it supports only 16 bits for the pid and therefore would generate wrong results on machines which have a pid >64k.

Also the pt_regs structure which was passed to oprofile common code cannot necessarily be used to generate sane backtraces, since the task(s) in question may run while the samples are fed to oprofile. So the result would be more or less random.

However given that the only user space tools switched to the perf interface already four years ago the hardware sampler code seems to be unused code, and therefore it should be reasonable to remove it.

The timer based oprofile support continues to work.

 Documentation/kernel-parameters.txt |    2 -
 arch/s390/oprofile/Makefile         |    1 -
 arch/s390/oprofile/hwsampler.c      | 1178 --------------------------------
 arch/s390/oprofile/hwsampler.h      |   63 --
 arch/s390/oprofile/init.c           |  489 -------------
 arch/s390/oprofile/op_counter.h     |   21 -
 6 files changed, 1754 deletions(-)
#6 lore raw
Quoted 26 lines · Heiko Carstens
Quoted 25 lines · Robert Richter

Heiko,

Quoted 10 lines · Heiko Carstens

However I'm wondering if we shouldn't simply remove at least the s390 specific hwswampler code from the oprofile module. This would still leave the common code timer based sampling mode for oprofile working on s390.

It looks like the oprofile user space utility nowadays (since 2012) uses the kernel perf interface instead of the oprofile interface anyway, if present. So the oprofile module itself doesn't seem to have too many users left.

Any opinions?

yes, the kernel driver is not necessary for oprofile userland for a while now. There is no ongoing development any longer, most patches are due to changes in the kernel apis.

So if there is code that needs a larger rework due to other kernel changes and there is no user anymore, I am fine with removing the code instead of reworking it. I still would just keep existing code as long as we can keep it unchanged (some like the lightwight of oprofile, esp. in the embedded space). If there is a user of the code, a Tested-by would be good for new code changes.

Quoted 5 lines
Quoted 2 lines

If there are users of the hwswampler, speak up now. Else, let's just remove it.

Ok, so I'll wait a week or so and remove the code if nobody speaks up. Is it ok for you if I add the patch to the s390 kernel tree?

Yes, pass it through your tree.

Quoted 20 lines

The patch would only remove s390 specific architecture code.

I have this pending:

s390/oprofile: remove hardware sampler support

Remove hardware sampler support from oprofile module.

The oprofile user space utilty has been switched to use the kernel perf interface, for which we also provide hardware sampling support.

In addition the hardware sampling support is also slightly broken: it supports only 16 bits for the pid and therefore would generate wrong results on machines which have a pid >64k.

Also the pt_regs structure which was passed to oprofile common code cannot necessarily be used to generate sane backtraces, since the task(s) in question may run while the samples are fed to oprofile. So the result would be more or less random.

Quoted 7 lines

However given that the only user space tools switched to the perf interface already four years ago the hardware sampler code seems to be unused code, and therefore it should be reasonable to remove it.

The timer based oprofile support continues to work.

Quoted 8 lines
 Documentation/kernel-parameters.txt |    2 -
 arch/s390/oprofile/Makefile         |    1 -
 arch/s390/oprofile/hwsampler.c      | 1178 --------------------------------
 arch/s390/oprofile/hwsampler.h      |   63 --
 arch/s390/oprofile/init.c           |  489 -------------
 arch/s390/oprofile/op_counter.h     |   21 -
 6 files changed, 1754 deletions(-)

Could you send the patch for review anyway?

Thanks,

-Robert

#7 lore raw
Quoted 10 lines · Robert Richter
Quoted 8 lines
 Documentation/kernel-parameters.txt |    2 -
 arch/s390/oprofile/Makefile         |    1 -
 arch/s390/oprofile/hwsampler.c      | 1178 --------------------------------
 arch/s390/oprofile/hwsampler.h      |   63 --
 arch/s390/oprofile/init.c           |  489 -------------
 arch/s390/oprofile/op_counter.h     |   21 -
 6 files changed, 1754 deletions(-)

Could you send the patch for review anyway?

Yes, of course. I added it below, hope that's ok.
[I did not yet add your Ack]

From 74ab0aefe07573358cc6a0c462ddc62ccba4c0cf Mon Sep 17 00:00:00 2001
From: Heiko Carstens <[email protected]>
Date: Fri, 10 Jun 2016 10:40:06 +0200
Subject: [PATCH] s390/oprofile: remove hardware sampler support

Remove hardware sampler support from oprofile module.

The oprofile user space utilty has been switched to use the kernel perf interface, for which we also provide hardware sampling support.

In addition the hardware sampling support is also slightly broken: it supports only 16 bits for the pid and therefore would generate wrong results on machines which have a pid >64k.

Also the pt_regs structure which was passed to oprofile common code cannot necessarily be used to generate sane backtraces, since the task(s) in question may run while the samples are fed to oprofile. So the result would be more or less random.

However given that the only user space tools switched to the perf interface already four years ago the hardware sampler code seems to be unused code, and therefore it should be reasonable to remove it.

The timer based oprofile support continues to work.

---
 Documentation/kernel-parameters.txt |    2 -
 arch/s390/oprofile/Makefile         |    1 -
 arch/s390/oprofile/hwsampler.c      | 1178 -----------------------------------
 arch/s390/oprofile/hwsampler.h      |   63 --
 arch/s390/oprofile/init.c           |  489 ---------------
 arch/s390/oprofile/op_counter.h     |   21 -
 6 files changed, 1754 deletions(-)
 delete mode 100644 arch/s390/oprofile/hwsampler.c
 delete mode 100644 arch/s390/oprofile/hwsampler.h
 delete mode 100644 arch/s390/oprofile/op_counter.h
Documentation/kernel-parameters.txt +0 -2
index 82b42c958d1c..68b0c022861a 100644
@@ -2788,8 +2788,6 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			timer: [X86] Force use of architectural NMI
 				timer mode (see also oprofile.timer
 				for generic hr timer mode)
-				[s390] Force legacy basic mode sampling
-                                (report cpu_type "timer")
 
 	oops=panic	Always panic on oopses. Default is to just kill the
 			process, but there is a small probability of
arch/s390/oprofile/Makefile +0 -1
index 496e4a7ee00e..e9dd41b0b8d3 100644
@@ -7,4 +7,3 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
 		timer_int.o )
 
 oprofile-y :=	$(DRIVER_OBJS) init.o
-oprofile-y +=	hwsampler.o
arch/s390/oprofile/hwsampler.c deleted +0 -1178

1,178 lines not on this page · raw message

arch/s390/oprofile/hwsampler.h deleted +0 -63

63 lines not on this page · raw message

arch/s390/oprofile/init.c +0 -489

506 lines not on this page · raw message

arch/s390/oprofile/op_counter.h deleted +0 -21
deleted file mode 100644
index 61b2531eef17..000000000000
@@ -1,21 +0,0 @@
-/*
- *   Copyright IBM Corp. 2011
- *   Author(s): Andreas Krebbel ([email protected])
- *
- * @remark Copyright 2011 OProfile authors
- */
-
-#ifndef OP_COUNTER_H
-#define OP_COUNTER_H
-
-struct op_counter_config {
-	/* `enabled' maps to the hwsampler_file variable.  */
-	/* `count' maps to the oprofile_hw_interval variable.  */
-	/* `event' and `unit_mask' are unused. */
-	unsigned long kernel;
-	unsigned long user;
-};
-
-extern struct op_counter_config counter_config;
-
-#endif /* OP_COUNTER_H */
Signature · 1 line
2.6.6
Reviewed-by
#8 lore raw
Quoted 41 lines · Heiko Carstens
Quoted 10 lines · Robert Richter
Quoted 8 lines
 Documentation/kernel-parameters.txt |    2 -
 arch/s390/oprofile/Makefile         |    1 -
 arch/s390/oprofile/hwsampler.c      | 1178 --------------------------------
 arch/s390/oprofile/hwsampler.h      |   63 --
 arch/s390/oprofile/init.c           |  489 -------------
 arch/s390/oprofile/op_counter.h     |   21 -
 6 files changed, 1754 deletions(-)

Could you send the patch for review anyway?

Yes, of course. I added it below, hope that's ok.
[I did not yet add your Ack]

From 74ab0aefe07573358cc6a0c462ddc62ccba4c0cf Mon Sep 17 00:00:00 2001
From: Heiko Carstens <[email protected]>
Date: Fri, 10 Jun 2016 10:40:06 +0200
Subject: [PATCH] s390/oprofile: remove hardware sampler support

Remove hardware sampler support from oprofile module.

The oprofile user space utilty has been switched to use the kernel perf interface, for which we also provide hardware sampling support.

In addition the hardware sampling support is also slightly broken: it supports only 16 bits for the pid and therefore would generate wrong results on machines which have a pid >64k.

Also the pt_regs structure which was passed to oprofile common code cannot necessarily be used to generate sane backtraces, since the task(s) in question may run while the samples are fed to oprofile. So the result would be more or less random.

However given that the only user space tools switched to the perf interface already four years ago the hardware sampler code seems to be unused code, and therefore it should be reasonable to remove it.

The timer based oprofile support continues to work.

Quoted 12 lines
---
 Documentation/kernel-parameters.txt |    2 -
 arch/s390/oprofile/Makefile         |    1 -
 arch/s390/oprofile/hwsampler.c      | 1178 -----------------------------------
 arch/s390/oprofile/hwsampler.h      |   63 --
 arch/s390/oprofile/init.c           |  489 ---------------
 arch/s390/oprofile/op_counter.h     |   21 -
 6 files changed, 1754 deletions(-)
 delete mode 100644 arch/s390/oprofile/hwsampler.c
 delete mode 100644 arch/s390/oprofile/hwsampler.h
 delete mode 100644 arch/s390/oprofile/op_counter.h