On Mon, 8 Aug 2022 03:41:27 +0100
"Matthew Wilcox (Oracle)" <[email protected]> wrote:
>
> @@ -9826,20 +9821,8 @@ static struct notifier_block trace_die_notifier = {
> void
> trace_printk_seq(struct trace_seq *s)
> {
> - /* Probably should print a warning here. */
> - if (s->seq.len >= TRACE_MAX_PRINT)
> - s->seq.len = TRACE_MAX_PRINT;
> -
> - /*
> - * More paranoid code. Although the buffer size is set to
> - * PAGE_SIZE, and TRACE_MAX_PRINT is 1000, this is just
> - * an extra layer of protection.
> - */
> - if (WARN_ON_ONCE(s->seq.len >= s->seq.size))
> - s->seq.len = s->seq.size - 1;
> -
> /* should be zero ended, but we are paranoid. */
> - s->buffer[s->seq.len] = 0;
> + printbuf_nul_terminate(&s->seq);
>
> printk(KERN_TRACE "%s", s->buffer);
>
> diff --g
Please remove the two tracing patches and the deletion of the seq_buf from
the series.
Thank you.
-- Steve
On 8/7/22 22:51, Steven Rostedt wrote:
> On Mon, 8 Aug 2022 03:41:27 +0100
> "Matthew Wilcox (Oracle)" <[email protected]> wrote:
>
>>
>> @@ -9826,20 +9821,8 @@ static struct notifier_block trace_die_notifier = {
>> void
>> trace_printk_seq(struct trace_seq *s)
>> {
>> - /* Probably should print a warning here. */
>> - if (s->seq.len >= TRACE_MAX_PRINT)
>> - s->seq.len = TRACE_MAX_PRINT;
>> -
>> - /*
>> - * More paranoid code. Although the buffer size is set to
>> - * PAGE_SIZE, and TRACE_MAX_PRINT is 1000, this is just
>> - * an extra layer of protection.
>> - */
>> - if (WARN_ON_ONCE(s->seq.len >= s->seq.size))
>> - s->seq.len = s->seq.size - 1;
>> -
>> /* should be zero ended, but we are paranoid. */
>> - s->buffer[s->seq.len] = 0;
>> + printbuf_nul_terminate(&s->seq);
>>
>> printk(KERN_TRACE "%s", s->buffer);
>>
>> diff --g
>
> Please remove the two tracing patches and the deletion of the seq_buf from
> the series.
Well, that's not really an option, as Christoph already (rightly)
pointed out.
If you've got actual engineering concerns that you'd care to articulate
I'd (still) like to try to work with you - otherwise, I don't think this
is something I can accommodate you on.
On Sun, 7 Aug 2022 23:32:01 -0400
Kent Overstreet <[email protected]> wrote:
> > Please remove the two tracing patches and the deletion of the seq_buf from
> > the series.
>
> Well, that's not really an option, as Christoph already (rightly)
> pointed out.
These are the last patches of the series. There's no dependency on them.
You should be able to simply drop them. If others find that these patches
are worth their while then by all means, let them have them.
>
> If you've got actual engineering concerns that you'd care to articulate
> I'd (still) like to try to work with you - otherwise, I don't think this
> is something I can accommodate you on.
Here's my technical reason. These are non trivial changes that are replacing
code that has been stable for 8 years that the tracing infrastructure
highly depends on. I do not have the time to sit down and review this code
as it is not a priority. My time is extremely limited (as my wife keeps
complaining to me about, as I'm not spending enough time with the family).
This change is likely to cause subtle regressions for no benefit to the
tracing subsystem. Hence, when it comes to risk vs reward, I see none.
I'm not NACKing the series, just the changes to the tracing subsystem.
When this code is mature enough, I may reconsider my stance on this.
-- Steve
On 8/8/22 09:37, Steven Rostedt wrote:
> On Sun, 7 Aug 2022 23:32:01 -0400
> Kent Overstreet <[email protected]> wrote:
>
>>> Please remove the two tracing patches and the deletion of the seq_buf from
>>> the series.
>>
>> Well, that's not really an option, as Christoph already (rightly)
>> pointed out.
>
> These are the last patches of the series. There's no dependency on them.
> You should be able to simply drop them. If others find that these patches
> are worth their while then by all means, let them have them.
>
>>
>> If you've got actual engineering concerns that you'd care to articulate
>> I'd (still) like to try to work with you - otherwise, I don't think this
>> is something I can accommodate you on.
>
> Here's my technical reason. These are non trivial changes that are replacing
> code that has been stable for 8 years that the tracing infrastructure
> highly depends on. I do not have the time to sit down and review this code
> as it is not a priority. My time is extremely limited (as my wife keeps
> complaining to me about, as I'm not spending enough time with the family).
>
> This change is likely to cause subtle regressions for no benefit to the
> tracing subsystem. Hence, when it comes to risk vs reward, I see none.
It sounds like you're saying you don't have time to maintain your
subsystem..? Is there anyone else actively co-maintaining tracing? Part
of our jobs is bringing new people in and training them (and not
providing a hostile work environment so they'll want to), maybe
something to think about.
I'm also not seeing the likelihood of subtle regressions - this isn't my
first kernel refactoring and not _nearly_ the biggest or the most
invasive. There's definitely some stuff in the tracing code code that is
a bit on the unorthodox side, but nothing too crazy. The code's been in
my tree for ages where I use tracing on a daily basis, and it passes
your test suite (and there was just one bug that made it through to be
caught by the tests, as I mentioned in the cover letter).
Anyways, if you've got specific, actionable concerns, I'll be happy to
take a look. Otherwise... ¯\_(ツ)_/¯
Cheers
On Mon, 8 Aug 2022 11:15:31 -0400
Kent Overstreet <[email protected]> wrote:
> > This change is likely to cause subtle regressions for no benefit to the
> > tracing subsystem. Hence, when it comes to risk vs reward, I see none.
>
> It sounds like you're saying you don't have time to maintain your
> subsystem..? Is there anyone else actively co-maintaining tracing? Part
> of our jobs is bringing new people in and training them (and not
> providing a hostile work environment so they'll want to), maybe
> something to think about.
No, it sounds like there's nothing here I need. Why do I need to review any
code that is not going to improve my subsystem?
>
> I'm also not seeing the likelihood of subtle regressions - this isn't my
> first kernel refactoring and not _nearly_ the biggest or the most
> invasive. There's definitely some stuff in the tracing code code that is
> a bit on the unorthodox side, but nothing too crazy. The code's been in
> my tree for ages where I use tracing on a daily basis, and it passes
> your test suite (and there was just one bug that made it through to be
> caught by the tests, as I mentioned in the cover letter).
>
> Anyways, if you've got specific, actionable concerns, I'll be happy to
> take a look. Otherwise... ¯\_(ツ)_/¯
Like I said. I don't see the improvement. I hate changes for changes sake
alone. If there was a real improvement to the system, then I would make the
time to look at it. But currently, the only thing I get is that you want
this code in. And that's not a high enough bar.
As I stated before, and have given talks about. Changes are pulled into
Linux, they are never pushed.
-- Steve