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 <[email protected]>
Signed-off-by: Valentin Schneider <[email protected]>
---
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;
+ 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:
--
2.31.1
On Thu, 20 Jul 2023 17:30:42 +0100
Valentin Schneider <[email protected]> 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 <[email protected]>
> Signed-off-by: Valentin Schneider <[email protected]>
> ---
> 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:
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
On 29/07/23 15:55, Steven Rostedt wrote:
> On Thu, 20 Jul 2023 17:30:42 +0100
> Valentin Schneider <[email protected]> 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 <[email protected]>
>> Signed-off-by: Valentin Schneider <[email protected]>
>> ---
>> 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:
On Mon, 31 Jul 2023 15:07:52 +0300
Dan Carpenter <[email protected]> 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
On Mon, Jul 31, 2023 at 11:54:53AM -0400, Steven Rostedt wrote:
> On Mon, 31 Jul 2023 15:07:52 +0300
> Dan Carpenter <[email protected]> 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
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 <[email protected]> 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
On Mon, 31 Jul 2023 19:03:04 +0300
Dan Carpenter <[email protected]> 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