diff mbox series

[RFC,v2,06/20] tracing/filters: Optimise scalar vs cpumask filtering when the user mask is a single CPU

Message ID 20230720163056.2564824-7-vschneid@redhat.com
State New
Headers
Series context_tracking,x86: Defer some IPIs until a user->kernel transition |

Commit Message

Valentin Schneider July 20, 2023, 4:30 p.m. UTC 
  Steven noted that when the user-provided cpumask contains a single CPU,
then the filtering function can use a scalar as input instead of a
full-fledged cpumask.

When the mask contains a single CPU, directly re-use the unsigned field
predicate functions. Transform '&' into '==' beforehand.

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---
 kernel/trace/trace_events_filter.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Steven Rostedt July 29, 2023, 7:55 p.m. UTC | #1 
On Thu, 20 Jul 2023 17:30:42 +0100
Valentin Schneider <vschneid@redhat.com> wrote:

> Steven noted that when the user-provided cpumask contains a single CPU,
> then the filtering function can use a scalar as input instead of a
> full-fledged cpumask.
> 
> When the mask contains a single CPU, directly re-use the unsigned field
> predicate functions. Transform '&' into '==' beforehand.
> 
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Valentin Schneider <vschneid@redhat.com>
> ---
>  kernel/trace/trace_events_filter.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 2fe65ddeb34ef..54d642fabb7f1 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -1750,7 +1750,7 @@ static int parse_pred(const char *str, void *data,
>  		 * then we can treat it as a scalar input.
>  		 */
>  		single = cpumask_weight(pred->mask) == 1;
> -		if (single && field->filter_type == FILTER_CPUMASK) {
> +		if (single && field->filter_type != FILTER_CPU) {
>  			pred->val = cpumask_first(pred->mask);
>  			kfree(pred->mask);
>  		}
> @@ -1761,6 +1761,11 @@ static int parse_pred(const char *str, void *data,
>  				FILTER_PRED_FN_CPUMASK;
>  		} else if (field->filter_type == FILTER_CPU) {
>  			pred->fn_num = FILTER_PRED_FN_CPU_CPUMASK;
> +		} else if (single) {
> +			pred->op = pred->op == OP_BAND ? OP_EQ : pred->op;

Nit, the above can be written as:

			pred->op = pret->op != OP_BAND ? : OP_EQ;

-- Steve


> +			pred->fn_num = select_comparison_fn(pred->op, field->size, false);
> +			if (pred->op == OP_NE)
> +				pred->not = 1;
>  		} else {
>  			switch (field->size) {
>  			case 8:
Valentin Schneider July 31, 2023, 11:20 a.m. UTC | #2 
On 29/07/23 15:55, Steven Rostedt wrote:
> On Thu, 20 Jul 2023 17:30:42 +0100
> Valentin Schneider <vschneid@redhat.com> wrote:
>
>> Steven noted that when the user-provided cpumask contains a single CPU,
>> then the filtering function can use a scalar as input instead of a
>> full-fledged cpumask.
>>
>> When the mask contains a single CPU, directly re-use the unsigned field
>> predicate functions. Transform '&' into '==' beforehand.
>>
>> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
>> Signed-off-by: Valentin Schneider <vschneid@redhat.com>
>> ---
>>  kernel/trace/trace_events_filter.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
>> index 2fe65ddeb34ef..54d642fabb7f1 100644
>> --- a/kernel/trace/trace_events_filter.c
>> +++ b/kernel/trace/trace_events_filter.c
>> @@ -1750,7 +1750,7 @@ static int parse_pred(const char *str, void *data,
>>               * then we can treat it as a scalar input.
>>               */
>>              single = cpumask_weight(pred->mask) == 1;
>> -		if (single && field->filter_type == FILTER_CPUMASK) {
>> +		if (single && field->filter_type != FILTER_CPU) {
>>                      pred->val = cpumask_first(pred->mask);
>>                      kfree(pred->mask);
>>              }
>> @@ -1761,6 +1761,11 @@ static int parse_pred(const char *str, void *data,
>>                              FILTER_PRED_FN_CPUMASK;
>>              } else if (field->filter_type == FILTER_CPU) {
>>                      pred->fn_num = FILTER_PRED_FN_CPU_CPUMASK;
>> +		} else if (single) {
>> +			pred->op = pred->op == OP_BAND ? OP_EQ : pred->op;
>
> Nit, the above can be written as:
>
>                       pred->op = pret->op != OP_BAND ? : OP_EQ;
>

That's neater, thanks!

> -- Steve
>
>
>> +			pred->fn_num = select_comparison_fn(pred->op, field->size, false);
>> +			if (pred->op == OP_NE)
>> +				pred->not = 1;
>>              } else {
>>                      switch (field->size) {
>>                      case 8:
Dan Carpenter July 31, 2023, 12:07 p.m. UTC | #3 
On Sat, Jul 29, 2023 at 03:55:47PM -0400, Steven Rostedt wrote:
> > @@ -1761,6 +1761,11 @@ static int parse_pred(const char *str, void *data,
> >  				FILTER_PRED_FN_CPUMASK;
> >  		} else if (field->filter_type == FILTER_CPU) {
> >  			pred->fn_num = FILTER_PRED_FN_CPU_CPUMASK;
> > +		} else if (single) {
> > +			pred->op = pred->op == OP_BAND ? OP_EQ : pred->op;
> 
> Nit, the above can be written as:
> 
> 			pred->op = pret->op != OP_BAND ? : OP_EQ;
> 

Heh.  Those are not equivalent.  The right way to write this is:

	if (pred->op == OP_BAND)
		pred->op = OP_EQ;

regards,
dan carpenter
Steven Rostedt July 31, 2023, 3:54 p.m. UTC | #4 
On Mon, 31 Jul 2023 15:07:52 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Sat, Jul 29, 2023 at 03:55:47PM -0400, Steven Rostedt wrote:
> > > @@ -1761,6 +1761,11 @@ static int parse_pred(const char *str, void *data,
> > >  				FILTER_PRED_FN_CPUMASK;
> > >  		} else if (field->filter_type == FILTER_CPU) {
> > >  			pred->fn_num = FILTER_PRED_FN_CPU_CPUMASK;
> > > +		} else if (single) {
> > > +			pred->op = pred->op == OP_BAND ? OP_EQ : pred->op;  
> > 
> > Nit, the above can be written as:
> > 
> > 			pred->op = pret->op != OP_BAND ? : OP_EQ;
> >   
> 
> Heh.  Those are not equivalent.  The right way to write this is:

You mean because of my typo?

> 
> 	if (pred->op == OP_BAND)
> 		pred->op = OP_EQ;

But sure, I'm fine with that, and it's probably more readable too.

-- Steve
Dan Carpenter July 31, 2023, 4:03 p.m. UTC | #5 
On Mon, Jul 31, 2023 at 11:54:53AM -0400, Steven Rostedt wrote:
> On Mon, 31 Jul 2023 15:07:52 +0300
> Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> > On Sat, Jul 29, 2023 at 03:55:47PM -0400, Steven Rostedt wrote:
> > > > @@ -1761,6 +1761,11 @@ static int parse_pred(const char *str, void *data,
> > > >  				FILTER_PRED_FN_CPUMASK;
> > > >  		} else if (field->filter_type == FILTER_CPU) {
> > > >  			pred->fn_num = FILTER_PRED_FN_CPU_CPUMASK;
> > > > +		} else if (single) {
> > > > +			pred->op = pred->op == OP_BAND ? OP_EQ : pred->op;  
> > > 
> > > Nit, the above can be written as:
> > > 
> > > 			pred->op = pret->op != OP_BAND ? : OP_EQ;
> > >   
> > 
> > Heh.  Those are not equivalent.  The right way to write this is:
> 
> You mean because of my typo?

No, I hadn't seen the s/pred/pret/ typo.  Your code does:

	if (pred->op != OP_BAND)
		pred->op = true;
	else
		pred->op OP_EQ;

Realy we should probably trigger a static checker warning any time
someone does a compare operations as part of a "x = comparison ?: bar;
Years ago, someone asked me to do that with regards to error codes like:

	return ret < 0 ?: -EINVAL;

but I don't remember the results.

regards,
dan carpenter
Valentin Schneider July 31, 2023, 5:20 p.m. UTC | #6 
On 31/07/23 19:03, Dan Carpenter wrote:
> On Mon, Jul 31, 2023 at 11:54:53AM -0400, Steven Rostedt wrote:
>> On Mon, 31 Jul 2023 15:07:52 +0300
>> Dan Carpenter <dan.carpenter@linaro.org> wrote:
>>
>> > On Sat, Jul 29, 2023 at 03:55:47PM -0400, Steven Rostedt wrote:
>> > > > @@ -1761,6 +1761,11 @@ static int parse_pred(const char *str, void *data,
>> > > >                                FILTER_PRED_FN_CPUMASK;
>> > > >                } else if (field->filter_type == FILTER_CPU) {
>> > > >                        pred->fn_num = FILTER_PRED_FN_CPU_CPUMASK;
>> > > > +		} else if (single) {
>> > > > +			pred->op = pred->op == OP_BAND ? OP_EQ : pred->op;
>> > >
>> > > Nit, the above can be written as:
>> > >
>> > >                  pred->op = pret->op != OP_BAND ? : OP_EQ;
>> > >
>> >
>> > Heh.  Those are not equivalent.  The right way to write this is:
>>
>> You mean because of my typo?
>
> No, I hadn't seen the s/pred/pret/ typo.  Your code does:
>
>       if (pred->op != OP_BAND)
>               pred->op = true;
>       else
>               pred->op OP_EQ;
>
> Realy we should probably trigger a static checker warning any time
> someone does a compare operations as part of a "x = comparison ?: bar;
> Years ago, someone asked me to do that with regards to error codes like:
>
>       return ret < 0 ?: -EINVAL;
>
> but I don't remember the results.
>

FWIW this is caught by GCC:

     error: the omitted middle operand in ?: will always be ‘true’, suggest explicit middle operand [-Werror=parentheses]
     pred->op = pred->op != OP_BAND ? : OP_EQ;


> regards,
> dan carpenter
Steven Rostedt July 31, 2023, 6:16 p.m. UTC | #7 
On Mon, 31 Jul 2023 19:03:04 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> > > > Nit, the above can be written as:
> > > > 
> > > > 			pred->op = pret->op != OP_BAND ? : OP_EQ;
> > > >     
> > > 
> > > Heh.  Those are not equivalent.  The right way to write this is:  
> > 
> > You mean because of my typo?  
> 
> No, I hadn't seen the s/pred/pret/ typo.  Your code does:
> 
> 	if (pred->op != OP_BAND)
> 		pred->op = true;
> 	else
> 		pred->op OP_EQ;

Ah, for some reason I was thinking the ? : just was just a nop, but I guess
it is to assign the cond value :-/

But of course every place I've done that, it was the condition value I
wanted, which was the same as the value being assigned.

Thanks,

-- Steve
diff mbox series

Patch

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 2fe65ddeb34ef..54d642fabb7f1 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1750,7 +1750,7 @@  static int parse_pred(const char *str, void *data,
 		 * then we can treat it as a scalar input.
 		 */
 		single = cpumask_weight(pred->mask) == 1;
-		if (single && field->filter_type == FILTER_CPUMASK) {
+		if (single && field->filter_type != FILTER_CPU) {
 			pred->val = cpumask_first(pred->mask);
 			kfree(pred->mask);
 		}
@@ -1761,6 +1761,11 @@  static int parse_pred(const char *str, void *data,
 				FILTER_PRED_FN_CPUMASK;
 		} else if (field->filter_type == FILTER_CPU) {
 			pred->fn_num = FILTER_PRED_FN_CPU_CPUMASK;
+		} else if (single) {
+			pred->op = pred->op == OP_BAND ? OP_EQ : pred->op;
+			pred->fn_num = select_comparison_fn(pred->op, field->size, false);
+			if (pred->op == OP_NE)
+				pred->not = 1;
 		} else {
 			switch (field->size) {
 			case 8: