diff mbox series

[net-next,3/3] net/sched: act_pedit: rate limit datapath messages

Message ID 20230309185158.310994-4-pctammela@mojatatu.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net/sched: act_pedit: minor improvements | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 20 this patch: 20
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 20 this patch: 20
netdev/checkpatch warning WARNING: line length of 105 exceeds 80 columns WARNING: line length of 107 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pedro Tammela March 9, 2023, 6:51 p.m. UTC 
Unbounded info messages in the pedit datapath can flood the printk ring buffer quite easily
depending on the action created. As these messages are informational, usually printing
some, not all, is enough to bring attention to the real issue.

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

Simon Horman March 10, 2023, 2:21 p.m. UTC | #1 
On Thu, Mar 09, 2023 at 03:51:58PM -0300, Pedro Tammela wrote:
> Unbounded info messages in the pedit datapath can flood the printk ring buffer quite easily
> depending on the action created. As these messages are informational, usually printing
> some, not all, is enough to bring attention to the real issue.

Would this reasoning also apply to other TC actions?

> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> ---
>  net/sched/act_pedit.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index e42cbfc369ff..b5a8fc19ee55 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -388,9 +388,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>  		}
>  
>  		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
> -		if (rc) {
> -			pr_info("tc action pedit bad header type specified (0x%x)\n",
> -				htype);
> +		if (unlikely(rc)) {

Do you really need unlikely() here (and no where else?)

> +			pr_info_ratelimited("tc action pedit bad header type specified (0x%x)\n", htype);
>  			goto bad;
>  		}
>  

...
Pedro Tammela March 13, 2023, 6:24 p.m. UTC | #2 
On 10/03/2023 11:21, Simon Horman wrote:
> On Thu, Mar 09, 2023 at 03:51:58PM -0300, Pedro Tammela wrote:
>> Unbounded info messages in the pedit datapath can flood the printk ring buffer quite easily
>> depending on the action created. As these messages are informational, usually printing
>> some, not all, is enough to bring attention to the real issue.
> 
> Would this reasoning also apply to other TC actions?

Hi Simon,

So far, the only action that has datapath pr_info() messages is pedit.
This seems like it comes from the old days, according to git.

> 
>> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
>> ---
>>   net/sched/act_pedit.c | 17 +++++++----------
>>   1 file changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
>> index e42cbfc369ff..b5a8fc19ee55 100644
>> --- a/net/sched/act_pedit.c
>> +++ b/net/sched/act_pedit.c
>> @@ -388,9 +388,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>>   		}
>>   
>>   		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
>> -		if (rc) {
>> -			pr_info("tc action pedit bad header type specified (0x%x)\n",
>> -				htype);
>> +		if (unlikely(rc)) {
> 
> Do you really need unlikely() here (and no where else?)

This case in particular is already checked in the netlink parsing code 
on create/update.
I was gonna delete the condition initially but then thought of hiding it 
under an unlikely branch.
As for the other branches, I didn't see much of a reason.

> 
>> +			pr_info_ratelimited("tc action pedit bad header type specified (0x%x)\n", htype);
>>   			goto bad;
>>   		}
>>   
> 
> ...
Simon Horman March 14, 2023, 3:26 p.m. UTC | #3 
On Mon, Mar 13, 2023 at 03:24:47PM -0300, Pedro Tammela wrote:
> On 10/03/2023 11:21, Simon Horman wrote:
> > On Thu, Mar 09, 2023 at 03:51:58PM -0300, Pedro Tammela wrote:
> > > Unbounded info messages in the pedit datapath can flood the printk ring buffer quite easily
> > > depending on the action created. As these messages are informational, usually printing
> > > some, not all, is enough to bring attention to the real issue.
> > 
> > Would this reasoning also apply to other TC actions?
> 
> Hi Simon,
> 
> So far, the only action that has datapath pr_info() messages is pedit.
> This seems like it comes from the old days, according to git.

I'd be in favour of unifying things.
But perhaps that is a topic for another day.

> > > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> > > ---
> > >   net/sched/act_pedit.c | 17 +++++++----------
> > >   1 file changed, 7 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> > > index e42cbfc369ff..b5a8fc19ee55 100644
> > > --- a/net/sched/act_pedit.c
> > > +++ b/net/sched/act_pedit.c
> > > @@ -388,9 +388,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
> > >   		}
> > >   		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
> > > -		if (rc) {
> > > -			pr_info("tc action pedit bad header type specified (0x%x)\n",
> > > -				htype);
> > > +		if (unlikely(rc)) {
> > 
> > Do you really need unlikely() here (and no where else?)
> 
> This case in particular is already checked in the netlink parsing code on
> create/update.
> I was gonna delete the condition initially but then thought of hiding it
> under an unlikely branch.
> As for the other branches, I didn't see much of a reason.

TBH, I'd drop the unlikely() unless there is some performance data.
Perhaps you can drop the log entirely, can it occur given the checking
elsewhere?

> > 
> > > +			pr_info_ratelimited("tc action pedit bad header type specified (0x%x)\n", htype);
> > >   			goto bad;
> > >   		}
> > 
> > ...
>
diff mbox series

Patch

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index e42cbfc369ff..b5a8fc19ee55 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -388,9 +388,8 @@  TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 		}
 
 		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
-		if (rc) {
-			pr_info("tc action pedit bad header type specified (0x%x)\n",
-				htype);
+		if (unlikely(rc)) {
+			pr_info_ratelimited("tc action pedit bad header type specified (0x%x)\n", htype);
 			goto bad;
 		}
 
@@ -398,8 +397,8 @@  TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 			u8 *d, _d;
 
 			if (!offset_valid(skb, hoffset + tkey->at)) {
-				pr_info("tc action pedit 'at' offset %d out of bounds\n",
-					hoffset + tkey->at);
+				pr_info_ratelimited("tc action pedit 'at' offset %d out of bounds\n",
+						    hoffset + tkey->at);
 				goto bad;
 			}
 			d = skb_header_pointer(skb, hoffset + tkey->at,
@@ -409,14 +408,13 @@  TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 
 			offset += (*d & tkey->offmask) >> tkey->shift;
 			if (offset % 4) {
-				pr_info("tc action pedit offset must be on 32 bit boundaries\n");
+				pr_info_ratelimited("tc action pedit offset must be on 32 bit boundaries\n");
 				goto bad;
 			}
 		}
 
 		if (!offset_valid(skb, hoffset + offset)) {
-			pr_info("tc action pedit offset %d out of bounds\n",
-				hoffset + offset);
+			pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset);
 			goto bad;
 		}
 
@@ -433,8 +431,7 @@  TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 			val = (*ptr + tkey->val) & ~tkey->mask;
 			break;
 		default:
-			pr_info("tc action pedit bad command (%d)\n",
-				cmd);
+			pr_info_ratelimited("tc action pedit bad command (%d)\n", cmd);
 			goto bad;
 		}