| Message ID | 1531950487-24554-1-git-send-email-mathieu.poirier@linaro.org |
|---|---|
| Headers | show |
| Series | perf: Add ioctl for PMU driver configuration | expand |
On Wed, 18 Jul 2018 15:48:00 -0600 Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > This set adds the capability to communiate event specific configuration > to the PMU kernel driver using an ioctl(). The functionatlity is made > generic enough for anyone to use but is targeted at the identification > of CoreSight sinks when operating in CPU-wide trace scenarios. > > --- > Changes for V3: > . Return an error for CPU-wide scenarios while the feature is being > implemented (Kim) Hi, I'm giving this series a short test-drive on Juno. It yields success for the --per-thread case..: $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ --per-thread uname -a Linux juno 4.18.0-rc8-00011-gb82af52c4b35-dirty #147 SMP PREEMPT Thu Aug 9 11:20:37 CDT 2018 aarch64 GNU/Linux [ perf record: Woken up 0 times to write data ] Warning: AUX data lost 1 times out of 2! [ perf record: Captured and wrote 0.067 MB perf.data ] $ ..but not for CPU-wide?: $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ uname -a failed to mmap with 12 (Cannot allocate memory) $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ -C 0 uname -a failed to mmap with 12 (Cannot allocate memory) $ Thanks, Kim
On Mon, 13 Aug 2018 at 11:46, Kim Phillips <kim.phillips@arm.com> wrote: > > On Wed, 18 Jul 2018 15:48:00 -0600 > Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > > This set adds the capability to communiate event specific configuration > > to the PMU kernel driver using an ioctl(). The functionatlity is made > > generic enough for anyone to use but is targeted at the identification > > of CoreSight sinks when operating in CPU-wide trace scenarios. > > > > --- > > Changes for V3: > > . Return an error for CPU-wide scenarios while the feature is being > > implemented (Kim) > > Hi, I'm giving this series a short test-drive on Juno. Hi, > > It yields success for the --per-thread case..: > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ --per-thread uname -a > Linux juno 4.18.0-rc8-00011-gb82af52c4b35-dirty #147 SMP PREEMPT Thu Aug 9 11:20:37 CDT 2018 aarch64 GNU/Linux > [ perf record: Woken up 0 times to write data ] > Warning: > AUX data lost 1 times out of 2! > > [ perf record: Captured and wrote 0.067 MB perf.data ] > $ > > ..but not for CPU-wide?: > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ uname -a > failed to mmap with 12 (Cannot allocate memory) > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ -C 0 uname -a > failed to mmap with 12 (Cannot allocate memory) > $ This patchset is getting very old and a fair amount of things have changed since then. I'm hoping to be coming out with a new one shortly. Nonetheless the above is returning an error in CPU-wide scenarios while the feature is being implemented. Isn't what you requested or have I misunderstood your comment? > > Thanks, > > Kim
On Tue, 14 Aug 2018 10:15:56 -0600 Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > On Mon, 13 Aug 2018 at 11:46, Kim Phillips <kim.phillips@arm.com> wrote: > > It yields success for the --per-thread case..: > > > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ --per-thread uname -a > > Linux juno 4.18.0-rc8-00011-gb82af52c4b35-dirty #147 SMP PREEMPT Thu Aug 9 11:20:37 CDT 2018 aarch64 GNU/Linux > > [ perf record: Woken up 0 times to write data ] > > Warning: > > AUX data lost 1 times out of 2! > > > > [ perf record: Captured and wrote 0.067 MB perf.data ] > > $ > > > > ..but not for CPU-wide?: > > > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ uname -a > > failed to mmap with 12 (Cannot allocate memory) > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ -C 0 uname -a > > failed to mmap with 12 (Cannot allocate memory) > > $ > > This patchset is getting very old and a fair amount of things have > changed since then. I'm hoping to be coming out with a new one > shortly. Nonetheless the above is returning an error in CPU-wide > scenarios while the feature is being implemented. Isn't what you > requested or have I misunderstood your comment? No, sigh, I just automatically assumed the patchset would include CPU-wide support again. If it were being done that way, we'd all know that the feature(s) this patchset adds would be doing the right thing for that purpose, guaranteed. The other thing that's going on here is that I'm becoming numb to the loathsome "failed to mmap with 12 (Cannot allocate memory)" being returned no matter what the error is/was. E.g., an error that would indicate a sense of non-implementation would be much better appreciated than presumably what the above is doing, i.e., returning -ENOMEM. That, backed up with specific details in the form of human readable text in dmesg would be *most* welcome. Thanks, Kim
On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: > > On Tue, 14 Aug 2018 10:15:56 -0600 > Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > > On Mon, 13 Aug 2018 at 11:46, Kim Phillips <kim.phillips@arm.com> wrote: > > > It yields success for the --per-thread case..: > > > > > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ --per-thread uname -a > > > Linux juno 4.18.0-rc8-00011-gb82af52c4b35-dirty #147 SMP PREEMPT Thu Aug 9 11:20:37 CDT 2018 aarch64 GNU/Linux > > > [ perf record: Woken up 0 times to write data ] > > > Warning: > > > AUX data lost 1 times out of 2! > > > > > > [ perf record: Captured and wrote 0.067 MB perf.data ] > > > $ > > > > > > ..but not for CPU-wide?: > > > > > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ uname -a > > > failed to mmap with 12 (Cannot allocate memory) > > > $ sudo taskset -c 0 ./perf record -e cs_etm/@20010000.etf/ -C 0 uname -a > > > failed to mmap with 12 (Cannot allocate memory) > > > $ > > > > This patchset is getting very old and a fair amount of things have > > changed since then. I'm hoping to be coming out with a new one > > shortly. Nonetheless the above is returning an error in CPU-wide > > scenarios while the feature is being implemented. Isn't what you > > requested or have I misunderstood your comment? > > No, sigh, I just automatically assumed the patchset would include > CPU-wide support again. If it were being done that way, we'd all know > that the feature(s) this patchset adds would be doing the right thing > for that purpose, guaranteed. The patchset published on this list never had support for CPU-wide scenarios. This is only a preparatory step, the first one in a few more to come. Sending the whole thing in one go would be way too heavy and is not realistic. > > The other thing that's going on here is that I'm becoming numb to the > loathsome "failed to mmap with 12 (Cannot allocate memory)" being > returned no matter what the error is/was. E.g., an error that would > indicate a sense of non-implementation would be much better > appreciated than presumably what the above is doing, i.e., returning > -ENOMEM. That, backed up with specific details in the form of human > readable text in dmesg would be *most* welcome. As part of the refactoring of the code to support CPU-wide scenarios I intend to emit better diagnostic messages from the driver. Modifying rb_alloc_aux() to propagate the error message generated by the architecture specific PMUs doesn't look hard either and I _may_ get to it as part of this work. Mathieu > > Thanks, > > Kim
On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: > On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: > > The other thing that's going on here is that I'm becoming numb to the > > loathsome "failed to mmap with 12 (Cannot allocate memory)" being > > returned no matter what the error is/was. E.g., an error that would > > indicate a sense of non-implementation would be much better > > appreciated than presumably what the above is doing, i.e., returning > > -ENOMEM. That, backed up with specific details in the form of human > > readable text in dmesg would be *most* welcome. > > As part of the refactoring of the code to support CPU-wide scenarios I > intend to emit better diagnostic messages from the driver. Modifying > rb_alloc_aux() to propagate the error message generated by the > architecture specific PMUs doesn't look hard either and I _may_ get to > it as part of this work. For the record, I will continue to oppose PMU drivers that dump diagnostics about user-controlled input into dmesg, but the coresight drivers are yours so it's up to you and I won't get in the way! Will
On Wed, 15 Aug 2018 10:39:13 +0100 Will Deacon <will.deacon@arm.com> wrote: > On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: > > On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: > > > The other thing that's going on here is that I'm becoming numb to the > > > loathsome "failed to mmap with 12 (Cannot allocate memory)" being > > > returned no matter what the error is/was. E.g., an error that would > > > indicate a sense of non-implementation would be much better > > > appreciated than presumably what the above is doing, i.e., returning > > > -ENOMEM. That, backed up with specific details in the form of human > > > readable text in dmesg would be *most* welcome. > > > > As part of the refactoring of the code to support CPU-wide scenarios I > > intend to emit better diagnostic messages from the driver. Modifying > > rb_alloc_aux() to propagate the error message generated by the > > architecture specific PMUs doesn't look hard either and I _may_ get to > > it as part of this work. > > For the record, I will continue to oppose PMU drivers that dump diagnostics > about user-controlled input into dmesg, but the coresight drivers are yours > so it's up to you and I won't get in the way! That sounds technically self-contradicting to me. Why shouldn't coresight share the same policies as those used for PMU drivers? Or why not allow the individual vendor PMU driver authors control the level of user-friendliness of their own drivers? That being said, Matheiu, would you accept patches that make coresight more verbose in dmesg? Kim
On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote: > > On Wed, 15 Aug 2018 10:39:13 +0100 > Will Deacon <will.deacon@arm.com> wrote: > > > On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: > > > On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: > > > > The other thing that's going on here is that I'm becoming numb to the > > > > loathsome "failed to mmap with 12 (Cannot allocate memory)" being > > > > returned no matter what the error is/was. E.g., an error that would > > > > indicate a sense of non-implementation would be much better > > > > appreciated than presumably what the above is doing, i.e., returning > > > > -ENOMEM. That, backed up with specific details in the form of human > > > > readable text in dmesg would be *most* welcome. > > > > > > As part of the refactoring of the code to support CPU-wide scenarios I > > > intend to emit better diagnostic messages from the driver. Modifying > > > rb_alloc_aux() to propagate the error message generated by the > > > architecture specific PMUs doesn't look hard either and I _may_ get to > > > it as part of this work. > > > > For the record, I will continue to oppose PMU drivers that dump diagnostics > > about user-controlled input into dmesg, but the coresight drivers are yours > > so it's up to you and I won't get in the way! > > That sounds technically self-contradicting to me. Why shouldn't > coresight share the same policies as those used for PMU drivers? Or > why not allow the individual vendor PMU driver authors control the > level of user-friendliness of their own drivers? > > That being said, Matheiu, would you accept patches that make coresight > more verbose in dmesg? It depends on the issue you're hoping to address. I'd rather see the root cause of the problem fixed than adding temporary code. Suzuki added the ETR perf API and I'm currently working on CPU-wide scenarios. From there and with regards to what can happen in setup_aux(), we should have things covered. > > Kim
On 08/16/2018 08:28 PM, Mathieu Poirier wrote: > On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote: >> >> On Wed, 15 Aug 2018 10:39:13 +0100 >> Will Deacon <will.deacon@arm.com> wrote: >> >>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: >>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: >>>>> The other thing that's going on here is that I'm becoming numb to the >>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being >>>>> returned no matter what the error is/was. E.g., an error that would >>>>> indicate a sense of non-implementation would be much better >>>>> appreciated than presumably what the above is doing, i.e., returning >>>>> -ENOMEM. That, backed up with specific details in the form of human >>>>> readable text in dmesg would be *most* welcome. >>>> >>>> As part of the refactoring of the code to support CPU-wide scenarios I >>>> intend to emit better diagnostic messages from the driver. Modifying >>>> rb_alloc_aux() to propagate the error message generated by the >>>> architecture specific PMUs doesn't look hard either and I _may_ get to >>>> it as part of this work. >>> >>> For the record, I will continue to oppose PMU drivers that dump diagnostics >>> about user-controlled input into dmesg, but the coresight drivers are yours >>> so it's up to you and I won't get in the way! >> >> That sounds technically self-contradicting to me. Why shouldn't >> coresight share the same policies as those used for PMU drivers? Or >> why not allow the individual vendor PMU driver authors control the >> level of user-friendliness of their own drivers? >> >> That being said, Matheiu, would you accept patches that make coresight >> more verbose in dmesg? > > It depends on the issue you're hoping to address. I'd rather see the > root cause of the problem fixed than adding temporary code. Suzuki > added the ETR perf API and I'm currently working on CPU-wide > scenarios. From there and with regards to what can happen in > setup_aux(), we should have things covered. I think the main issue is the lack of error code propagation from setup_aux() back to the perf_aux_output_handle_begin(), which always return -ENOMEM. If we fix that, we could get better idea of whats wrong. If someone is planning to add verbose messages, they may do so by adding dev_dbg() / pr_debug(), which can be turned on as and when needed. Suzuki
On Mon, 20 Aug 2018 11:03:03 +0100 Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > On 08/16/2018 08:28 PM, Mathieu Poirier wrote: > > On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote: > >> > >> On Wed, 15 Aug 2018 10:39:13 +0100 > >> Will Deacon <will.deacon@arm.com> wrote: > >> > >>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: > >>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: > >>>>> The other thing that's going on here is that I'm becoming numb to the > >>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being > >>>>> returned no matter what the error is/was. E.g., an error that would > >>>>> indicate a sense of non-implementation would be much better > >>>>> appreciated than presumably what the above is doing, i.e., returning > >>>>> -ENOMEM. That, backed up with specific details in the form of human > >>>>> readable text in dmesg would be *most* welcome. > >>>> > >>>> As part of the refactoring of the code to support CPU-wide scenarios I > >>>> intend to emit better diagnostic messages from the driver. Modifying > >>>> rb_alloc_aux() to propagate the error message generated by the > >>>> architecture specific PMUs doesn't look hard either and I _may_ get to > >>>> it as part of this work. > >>> > >>> For the record, I will continue to oppose PMU drivers that dump diagnostics > >>> about user-controlled input into dmesg, but the coresight drivers are yours > >>> so it's up to you and I won't get in the way! > >> > >> That sounds technically self-contradicting to me. Why shouldn't > >> coresight share the same policies as those used for PMU drivers? Or > >> why not allow the individual vendor PMU driver authors control the > >> level of user-friendliness of their own drivers? > >> > >> That being said, Matheiu, would you accept patches that make coresight > >> more verbose in dmesg? > > > > It depends on the issue you're hoping to address. I'd rather see the > > root cause of the problem fixed than adding temporary code. Suzuki > > added the ETR perf API and I'm currently working on CPU-wide > > scenarios. From there and with regards to what can happen in > > setup_aux(), we should have things covered. > > I think the main issue is the lack of error code propagation from > setup_aux() back to the perf_aux_output_handle_begin(), which always > return -ENOMEM. If we fix that, we could get better idea of whats > wrong. Why get a better idea when we can get the exact details? > If someone is planning to add verbose messages, they may do so by adding > dev_dbg() / pr_debug(), which can be turned on as and when needed. I disagree: that just adds another usage and kernel configuration obstacle. Why not use pr_err straight up? Kim
On 08/20/2018 03:22 PM, Kim Phillips wrote: > On Mon, 20 Aug 2018 11:03:03 +0100 > Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > >> On 08/16/2018 08:28 PM, Mathieu Poirier wrote: >>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote: >>>> >>>> On Wed, 15 Aug 2018 10:39:13 +0100 >>>> Will Deacon <will.deacon@arm.com> wrote: >>>> >>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: >>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: >>>>>>> The other thing that's going on here is that I'm becoming numb to the >>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being >>>>>>> returned no matter what the error is/was. E.g., an error that would >>>>>>> indicate a sense of non-implementation would be much better >>>>>>> appreciated than presumably what the above is doing, i.e., returning >>>>>>> -ENOMEM. That, backed up with specific details in the form of human >>>>>>> readable text in dmesg would be *most* welcome. >>>>>> >>>>>> As part of the refactoring of the code to support CPU-wide scenarios I >>>>>> intend to emit better diagnostic messages from the driver. Modifying >>>>>> rb_alloc_aux() to propagate the error message generated by the >>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to >>>>>> it as part of this work. >>>>> >>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics >>>>> about user-controlled input into dmesg, but the coresight drivers are yours >>>>> so it's up to you and I won't get in the way! >>>> >>>> That sounds technically self-contradicting to me. Why shouldn't >>>> coresight share the same policies as those used for PMU drivers? Or >>>> why not allow the individual vendor PMU driver authors control the >>>> level of user-friendliness of their own drivers? >>>> >>>> That being said, Matheiu, would you accept patches that make coresight >>>> more verbose in dmesg? >>> >>> It depends on the issue you're hoping to address. I'd rather see the >>> root cause of the problem fixed than adding temporary code. Suzuki >>> added the ETR perf API and I'm currently working on CPU-wide >>> scenarios. From there and with regards to what can happen in >>> setup_aux(), we should have things covered. >> >> I think the main issue is the lack of error code propagation from >> setup_aux() back to the perf_aux_output_handle_begin(), which always >> return -ENOMEM. If we fix that, we could get better idea of whats >> wrong. > > Why get a better idea when we can get the exact details? The different values for error numbers are there for a reason... > >> If someone is planning to add verbose messages, they may do so by adding >> dev_dbg() / pr_debug(), which can be turned on as and when needed. > > I disagree: that just adds another usage and kernel configuration > obstacle. Why not use pr_err straight up? I personally don't agree to usage of pr_err() in paths which are easily triggered from user input. Also, we are moving all the "debugging" messages to the dynamic debug, to prevent lockdep splats. Suzuki
On Mon, 20 Aug 2018 15:36:47 +0100 Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > On 08/20/2018 03:22 PM, Kim Phillips wrote: > > On Mon, 20 Aug 2018 11:03:03 +0100 > > Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > > >> On 08/16/2018 08:28 PM, Mathieu Poirier wrote: > >>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote: > >>>> > >>>> On Wed, 15 Aug 2018 10:39:13 +0100 > >>>> Will Deacon <will.deacon@arm.com> wrote: > >>>> > >>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: > >>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: > >>>>>>> The other thing that's going on here is that I'm becoming numb to the > >>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being > >>>>>>> returned no matter what the error is/was. E.g., an error that would > >>>>>>> indicate a sense of non-implementation would be much better > >>>>>>> appreciated than presumably what the above is doing, i.e., returning > >>>>>>> -ENOMEM. That, backed up with specific details in the form of human > >>>>>>> readable text in dmesg would be *most* welcome. > >>>>>> > >>>>>> As part of the refactoring of the code to support CPU-wide scenarios I > >>>>>> intend to emit better diagnostic messages from the driver. Modifying > >>>>>> rb_alloc_aux() to propagate the error message generated by the > >>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to > >>>>>> it as part of this work. > >>>>> > >>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics > >>>>> about user-controlled input into dmesg, but the coresight drivers are yours > >>>>> so it's up to you and I won't get in the way! > >>>> > >>>> That sounds technically self-contradicting to me. Why shouldn't > >>>> coresight share the same policies as those used for PMU drivers? Or > >>>> why not allow the individual vendor PMU driver authors control the > >>>> level of user-friendliness of their own drivers? > >>>> > >>>> That being said, Matheiu, would you accept patches that make coresight > >>>> more verbose in dmesg? > >>> > >>> It depends on the issue you're hoping to address. I'd rather see the > >>> root cause of the problem fixed than adding temporary code. Suzuki > >>> added the ETR perf API and I'm currently working on CPU-wide > >>> scenarios. From there and with regards to what can happen in > >>> setup_aux(), we should have things covered. > >> > >> I think the main issue is the lack of error code propagation from > >> setup_aux() back to the perf_aux_output_handle_begin(), which always > >> return -ENOMEM. If we fix that, we could get better idea of whats > >> wrong. > > > > Why get a better idea when we can get the exact details? > > The different values for error numbers are there for a reason... But the same error number, e.g., EINVAL, can be returned for different reasons. > >> If someone is planning to add verbose messages, they may do so by adding > >> dev_dbg() / pr_debug(), which can be turned on as and when needed. > > > > I disagree: that just adds another usage and kernel configuration > > obstacle. Why not use pr_err straight up? > > I personally don't agree to usage of pr_err() in paths which are easily > triggered from user input. Why not? pr_* are ratelimited. > Also, we are moving all the "debugging" > messages to the dynamic debug, to prevent lockdep splats. Are you referring to this?: https://lkml.org/lkml/2018/5/1/73 Re-reading the thread, AFAICT, it was never proven that the splats occurred due to the dev_infos, and there's no dev_info in this stacktrace: https://lkml.org/lkml/2018/5/10/269 But even if it were, wouldn't the splats also occur with dev_dbg? Kim
On 08/20/2018 04:25 PM, Kim Phillips wrote: > On Mon, 20 Aug 2018 15:36:47 +0100 > Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > >> On 08/20/2018 03:22 PM, Kim Phillips wrote: >>> On Mon, 20 Aug 2018 11:03:03 +0100 >>> Suzuki K Poulose <suzuki.poulose@arm.com> wrote: >>> >>>> On 08/16/2018 08:28 PM, Mathieu Poirier wrote: >>>>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote: >>>>>> >>>>>> On Wed, 15 Aug 2018 10:39:13 +0100 >>>>>> Will Deacon <will.deacon@arm.com> wrote: >>>>>> >>>>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: >>>>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: >>>>>>>>> The other thing that's going on here is that I'm becoming numb to the >>>>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being >>>>>>>>> returned no matter what the error is/was. E.g., an error that would >>>>>>>>> indicate a sense of non-implementation would be much better >>>>>>>>> appreciated than presumably what the above is doing, i.e., returning >>>>>>>>> -ENOMEM. That, backed up with specific details in the form of human >>>>>>>>> readable text in dmesg would be *most* welcome. >>>>>>>> >>>>>>>> As part of the refactoring of the code to support CPU-wide scenarios I >>>>>>>> intend to emit better diagnostic messages from the driver. Modifying >>>>>>>> rb_alloc_aux() to propagate the error message generated by the >>>>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to >>>>>>>> it as part of this work. >>>>>>> >>>>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics >>>>>>> about user-controlled input into dmesg, but the coresight drivers are yours >>>>>>> so it's up to you and I won't get in the way! >>>>>> >>>>>> That sounds technically self-contradicting to me. Why shouldn't >>>>>> coresight share the same policies as those used for PMU drivers? Or >>>>>> why not allow the individual vendor PMU driver authors control the >>>>>> level of user-friendliness of their own drivers? >>>>>> >>>>>> That being said, Matheiu, would you accept patches that make coresight >>>>>> more verbose in dmesg? >>>>> >>>>> It depends on the issue you're hoping to address. I'd rather see the >>>>> root cause of the problem fixed than adding temporary code. Suzuki >>>>> added the ETR perf API and I'm currently working on CPU-wide >>>>> scenarios. From there and with regards to what can happen in >>>>> setup_aux(), we should have things covered. >>>> >>>> I think the main issue is the lack of error code propagation from >>>> setup_aux() back to the perf_aux_output_handle_begin(), which always >>>> return -ENOMEM. If we fix that, we could get better idea of whats >>>> wrong. >>> >>> Why get a better idea when we can get the exact details? >> >> The different values for error numbers are there for a reason... > > But the same error number, e.g., EINVAL, can be returned for different > reasons. True, but then you can narrow it down by tuning it. We do that for several syscalls without printing any useful messages to debug. Why should the perf syscalls be any different ? > >>>> If someone is planning to add verbose messages, they may do so by adding >>>> dev_dbg() / pr_debug(), which can be turned on as and when needed. >>> >>> I disagree: that just adds another usage and kernel configuration >>> obstacle. Why not use pr_err straight up? >> >> I personally don't agree to usage of pr_err() in paths which are easily >> triggered from user input. > > Why not? pr_* are ratelimited. > >> Also, we are moving all the "debugging" >> messages to the dynamic debug, to prevent lockdep splats. > > Are you referring to this?: > > https://lkml.org/lkml/2018/5/1/7 Definitely not that thread. > > Re-reading the thread, AFAICT, it was never proven that the splats > occurred due to the dev_infos, and there's no dev_info in this > stacktrace: > > https://lkml.org/lkml/2018/5/10/269 That doesn't have the stack trace. Mathieu was also able to reproduce the lockdep splat involving console lock lately. Unfortunately none of these were captured here. > > But even if it were, wouldn't the splats also occur with dev_dbg? Not normally. dev_dbg() has to be turned on manually and the user knows what he is doing. That allows the normal user to use the system without any trouble. Suzuki
On Tue, 21 Aug 2018 09:39:22 +0100 Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > On 08/20/2018 04:25 PM, Kim Phillips wrote: > > On Mon, 20 Aug 2018 15:36:47 +0100 > > Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > > >> On 08/20/2018 03:22 PM, Kim Phillips wrote: > >>> On Mon, 20 Aug 2018 11:03:03 +0100 > >>> Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > >>> > >>>> On 08/16/2018 08:28 PM, Mathieu Poirier wrote: > >>>>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote: > >>>>>> > >>>>>> On Wed, 15 Aug 2018 10:39:13 +0100 > >>>>>> Will Deacon <will.deacon@arm.com> wrote: > >>>>>> > >>>>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: > >>>>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: > >>>>>>>>> The other thing that's going on here is that I'm becoming numb to the > >>>>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being > >>>>>>>>> returned no matter what the error is/was. E.g., an error that would > >>>>>>>>> indicate a sense of non-implementation would be much better > >>>>>>>>> appreciated than presumably what the above is doing, i.e., returning > >>>>>>>>> -ENOMEM. That, backed up with specific details in the form of human > >>>>>>>>> readable text in dmesg would be *most* welcome. > >>>>>>>> > >>>>>>>> As part of the refactoring of the code to support CPU-wide scenarios I > >>>>>>>> intend to emit better diagnostic messages from the driver. Modifying > >>>>>>>> rb_alloc_aux() to propagate the error message generated by the > >>>>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to > >>>>>>>> it as part of this work. > >>>>>>> > >>>>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics > >>>>>>> about user-controlled input into dmesg, but the coresight drivers are yours > >>>>>>> so it's up to you and I won't get in the way! > >>>>>> > >>>>>> That sounds technically self-contradicting to me. Why shouldn't > >>>>>> coresight share the same policies as those used for PMU drivers? Or > >>>>>> why not allow the individual vendor PMU driver authors control the > >>>>>> level of user-friendliness of their own drivers? > >>>>>> > >>>>>> That being said, Matheiu, would you accept patches that make coresight > >>>>>> more verbose in dmesg? > >>>>> > >>>>> It depends on the issue you're hoping to address. I'd rather see the > >>>>> root cause of the problem fixed than adding temporary code. Suzuki > >>>>> added the ETR perf API and I'm currently working on CPU-wide > >>>>> scenarios. From there and with regards to what can happen in > >>>>> setup_aux(), we should have things covered. > >>>> > >>>> I think the main issue is the lack of error code propagation from > >>>> setup_aux() back to the perf_aux_output_handle_begin(), which always > >>>> return -ENOMEM. If we fix that, we could get better idea of whats > >>>> wrong. > >>> > >>> Why get a better idea when we can get the exact details? > >> > >> The different values for error numbers are there for a reason... > > > > But the same error number, e.g., EINVAL, can be returned for different > > reasons. > > True, but then you can narrow it down by tuning it. I'm trying to avoid having our users have to do that every time something isn't quite right :) > We do that for > several syscalls without printing any useful messages to debug. Why > should the perf syscalls be any different ? Not sure what syscalls you kernel hackers are talking about, nor whether the work was being done on behalf of an end-user, but things like mount have the same problem as perf, and perf is notorious for it. > >>>> If someone is planning to add verbose messages, they may do so by adding > >>>> dev_dbg() / pr_debug(), which can be turned on as and when needed. > >>> > >>> I disagree: that just adds another usage and kernel configuration > >>> obstacle. Why not use pr_err straight up? > >> > >> I personally don't agree to usage of pr_err() in paths which are easily > >> triggered from user input. > > > > Why not? pr_* are ratelimited. > > > >> Also, we are moving all the "debugging" > >> messages to the dynamic debug, to prevent lockdep splats. > > > > Are you referring to this?: > > > > https://lkml.org/lkml/2018/5/1/7 > > Definitely not that thread. bah, sorry, apparently the trailing 3 was removed somehow. Here you go: https://lkml.org/lkml/2018/5/1/73 > > Re-reading the thread, AFAICT, it was never proven that the splats > > occurred due to the dev_infos, and there's no dev_info in this > > stacktrace: > > > > https://lkml.org/lkml/2018/5/10/269 > > That doesn't have the stack trace. Mathieu was also able to reproduce > the lockdep splat involving console lock lately. Unfortunately none of > these were captured here. OK, I'd rather we see and properly debug and fix the so-called lockdep splats rather than hiding them by lowering their loglevel. > > But even if it were, wouldn't the splats also occur with dev_dbg? > > Not normally. dev_dbg() has to be turned on manually and the user knows > what he is doing. That allows the normal user to use the system without > any trouble. I thought it was obvious that I was talking about when dev_dbg _is_ enabled. There, the problem still occurs, so you can't say 'we are moving all the "debugging" messages to the dynamic debug, to prevent lockdep splats.' Please let's see the lockdep splat and the proper fix for it instead of paper-taping over it via dev_dbg, thanks. Kim
On Mon, 20 Aug 2018 at 08:36, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > On 08/20/2018 03:22 PM, Kim Phillips wrote: > > On Mon, 20 Aug 2018 11:03:03 +0100 > > Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > > >> On 08/16/2018 08:28 PM, Mathieu Poirier wrote: > >>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote: > >>>> > >>>> On Wed, 15 Aug 2018 10:39:13 +0100 > >>>> Will Deacon <will.deacon@arm.com> wrote: > >>>> > >>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: > >>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: > >>>>>>> The other thing that's going on here is that I'm becoming numb to the > >>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being > >>>>>>> returned no matter what the error is/was. E.g., an error that would > >>>>>>> indicate a sense of non-implementation would be much better > >>>>>>> appreciated than presumably what the above is doing, i.e., returning > >>>>>>> -ENOMEM. That, backed up with specific details in the form of human > >>>>>>> readable text in dmesg would be *most* welcome. > >>>>>> > >>>>>> As part of the refactoring of the code to support CPU-wide scenarios I > >>>>>> intend to emit better diagnostic messages from the driver. Modifying > >>>>>> rb_alloc_aux() to propagate the error message generated by the > >>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to > >>>>>> it as part of this work. > >>>>> > >>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics > >>>>> about user-controlled input into dmesg, but the coresight drivers are yours > >>>>> so it's up to you and I won't get in the way! > >>>> > >>>> That sounds technically self-contradicting to me. Why shouldn't > >>>> coresight share the same policies as those used for PMU drivers? Or > >>>> why not allow the individual vendor PMU driver authors control the > >>>> level of user-friendliness of their own drivers? > >>>> > >>>> That being said, Matheiu, would you accept patches that make coresight > >>>> more verbose in dmesg? > >>> > >>> It depends on the issue you're hoping to address. I'd rather see the > >>> root cause of the problem fixed than adding temporary code. Suzuki > >>> added the ETR perf API and I'm currently working on CPU-wide > >>> scenarios. From there and with regards to what can happen in > >>> setup_aux(), we should have things covered. > >> > >> I think the main issue is the lack of error code propagation from > >> setup_aux() back to the perf_aux_output_handle_begin(), which always > >> return -ENOMEM. If we fix that, we could get better idea of whats > >> wrong. > > > > Why get a better idea when we can get the exact details? > > The different values for error numbers are there for a reason... > > > > >> If someone is planning to add verbose messages, they may do so by adding > >> dev_dbg() / pr_debug(), which can be turned on as and when needed. > > > > I disagree: that just adds another usage and kernel configuration > > obstacle. Why not use pr_err straight up? I think everything on this topic has been said already. As I remarked earlier once I'm done with CPU-wide support we shouldn't need detailed error reporting. Everything should be handled via error code propagation and that is what I'd like to see addressed in the first place. From there we can think about individual error cases as they come up. > > I personally don't agree to usage of pr_err() in paths which are easily > triggered from user input. Also, we are moving all the "debugging" > messages to the dynamic debug, to prevent lockdep splats. A slight correction here - we are moving most of the framework error reporting to dynamic debug because they clog the log file and aren't useful outside of a development context. It is not a remedy for the negative interaction between event locking and console access generated by the framework's reporting of device status as a path is built/enabled/disabled. > > Suzuki
On Tue, 21 Aug 2018 11:16:49 -0600 Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > On Mon, 20 Aug 2018 at 08:36, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > > > On 08/20/2018 03:22 PM, Kim Phillips wrote: > > > On Mon, 20 Aug 2018 11:03:03 +0100 > > > Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > > > > >> On 08/16/2018 08:28 PM, Mathieu Poirier wrote: > > >>> On Wed, 15 Aug 2018 at 09:28, Kim Phillips <kim.phillips@arm.com> wrote: > > >>>> > > >>>> On Wed, 15 Aug 2018 10:39:13 +0100 > > >>>> Will Deacon <will.deacon@arm.com> wrote: > > >>>> > > >>>>> On Tue, Aug 14, 2018 at 01:42:27PM -0600, Mathieu Poirier wrote: > > >>>>>> On Tue, 14 Aug 2018 at 11:09, Kim Phillips <kim.phillips@arm.com> wrote: > > >>>>>>> The other thing that's going on here is that I'm becoming numb to the > > >>>>>>> loathsome "failed to mmap with 12 (Cannot allocate memory)" being > > >>>>>>> returned no matter what the error is/was. E.g., an error that would > > >>>>>>> indicate a sense of non-implementation would be much better > > >>>>>>> appreciated than presumably what the above is doing, i.e., returning > > >>>>>>> -ENOMEM. That, backed up with specific details in the form of human > > >>>>>>> readable text in dmesg would be *most* welcome. > > >>>>>> > > >>>>>> As part of the refactoring of the code to support CPU-wide scenarios I > > >>>>>> intend to emit better diagnostic messages from the driver. Modifying > > >>>>>> rb_alloc_aux() to propagate the error message generated by the > > >>>>>> architecture specific PMUs doesn't look hard either and I _may_ get to > > >>>>>> it as part of this work. > > >>>>> > > >>>>> For the record, I will continue to oppose PMU drivers that dump diagnostics > > >>>>> about user-controlled input into dmesg, but the coresight drivers are yours > > >>>>> so it's up to you and I won't get in the way! > > >>>> > > >>>> That sounds technically self-contradicting to me. Why shouldn't > > >>>> coresight share the same policies as those used for PMU drivers? Or > > >>>> why not allow the individual vendor PMU driver authors control the > > >>>> level of user-friendliness of their own drivers? > > >>>> > > >>>> That being said, Matheiu, would you accept patches that make coresight > > >>>> more verbose in dmesg? > > >>> > > >>> It depends on the issue you're hoping to address. I'd rather see the > > >>> root cause of the problem fixed than adding temporary code. Suzuki > > >>> added the ETR perf API and I'm currently working on CPU-wide > > >>> scenarios. From there and with regards to what can happen in > > >>> setup_aux(), we should have things covered. > > >> > > >> I think the main issue is the lack of error code propagation from > > >> setup_aux() back to the perf_aux_output_handle_begin(), which always > > >> return -ENOMEM. If we fix that, we could get better idea of whats > > >> wrong. > > > > > > Why get a better idea when we can get the exact details? > > > > The different values for error numbers are there for a reason... > > > > > > > >> If someone is planning to add verbose messages, they may do so by adding > > >> dev_dbg() / pr_debug(), which can be turned on as and when needed. > > > > > > I disagree: that just adds another usage and kernel configuration > > > obstacle. Why not use pr_err straight up? > > I think everything on this topic has been said already. As I remarked Apparently not :) > earlier once I'm done with CPU-wide support we shouldn't need detailed > error reporting. Everything should be handled via error code > propagation and that is what I'd like to see addressed in the first > place. From there we can think about individual error cases as they > come up. FYI, there are a lot of -EINVAL cases: $ git grep -- -EINVAL drivers/hwtracing/coresight/ | wc -l 142 > > I personally don't agree to usage of pr_err() in paths which are easily > > triggered from user input. Also, we are moving all the "debugging" > > messages to the dynamic debug, to prevent lockdep splats. > > A slight correction here - we are moving most of the framework error > reporting to dynamic debug because they clog the log file and aren't > useful outside of a development context. It is not a remedy for the I didn't see anyone complain about any 'clogging', and I sure don't mind seeing the extra messaging, esp. when using coresight in sysfs mode: it's reinforcement to the user that it's doing work. > negative interaction between event locking and console access > generated by the framework's reporting of device status as a path is > built/enabled/disabled. I'd like to see this splat from this 'negative interaction', and it be fixed before we allow code in that makes it silently go away... Kim
This set adds the capability to communiate event specific configuration to the PMU kernel driver using an ioctl(). The functionatlity is made generic enough for anyone to use but is targeted at the identification of CoreSight sinks when operating in CPU-wide trace scenarios. Applies cleanly on v4.18-rc5. Thanks, Mathieu --- Changes for V3: . Return an error for CPU-wide scenarios while the feature is being implemented (Kim) . Proper initialisation for event::hw::drv_config::lock (Kim) Changes for V2: . Fixed s390 problem reported by buildbot. . Removed uneeded check in perf_event_process_drv_config() (Jiri) . Reordered data copy in perf_event_set_drv_config() (Jiri) . Went from 2 to 1 step driver configuration process (Alex) . Moved structure name "perf_drv_config" to "pmu_drv_config". V1: https://lkml.org/lkml/2018/7/2/1008 Mathieu Poirier (7): perf: Introduce ioctl to communicate driver configuration to kernel perf/core: Use ioctl to communicate driver configuration to kernel perf/aux: Make perf_event accessible to setup_aux() coresight: Use PMU driver configuration for sink selection perf tools: Use ioctl to communicate driver configuration to kernel perf tools: Make perf_evsel accessible to PMU driver configuration code perf tools: Use ioctl function to send sink configuration to kernel arch/s390/kernel/perf_cpum_sf.c | 6 +- arch/x86/events/intel/bts.c | 4 +- arch/x86/events/intel/pt.c | 5 +- drivers/hwtracing/coresight/coresight-etm-perf.c | 140 ++++++++++++++++++++--- drivers/hwtracing/coresight/coresight-etm-perf.h | 4 + drivers/perf/arm_spe_pmu.c | 6 +- include/linux/perf_event.h | 47 +++++++- include/uapi/linux/perf_event.h | 1 + kernel/events/core.c | 78 +++++++++++++ kernel/events/ring_buffer.c | 2 +- tools/include/uapi/linux/perf_event.h | 1 + tools/perf/arch/arm/util/cs-etm.c | 60 +++------- tools/perf/arch/arm/util/cs-etm.h | 3 +- tools/perf/util/drv_configs.c | 30 ++--- tools/perf/util/evsel.c | 7 ++ tools/perf/util/evsel.h | 1 + tools/perf/util/pmu.h | 3 +- 17 files changed, 305 insertions(+), 93 deletions(-) -- 2.7.4