Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jiri Pirko]

Sun, Oct 29, 2017 at 08:26:25AM CET, j...@resnulli.us wrote:
>Sat, Oct 28, 2017 at 07:17:24PM CEST, kubak...@wp.pl wrote:
>>On Sat, 28 Oct 2017 10:43:51 +0200, Jiri Pirko wrote:
>>> Sat, Oct 28, 2017 at 09:53:21AM CEST, kubak...@wp.pl wrote:
>>> >On Sat, 28 Oct 2017 09:20:31 +0200, Jiri Pirko wrote:  
>>> >> Sat, Oct 28, 2017 at 02:52:00AM CEST, kubak...@wp.pl wrote:  
>>> >> >On Fri, 27 Oct 2017 09:27:30 +0200, Jiri Pirko wrote:
>>> >> >> Yes, it is the same.
>>> >> >
>>> >> >FWIW I also see what Amritha and Alex are describing here, for cls_bpf
>>> >> >there are no DESTROYs coming on rmmod or qdisc del.  There is a DESTROY
>>> >> >if I manually remove the filter (or if an ADD with skip_sw fails).
>>> >> 
>>> >> Is this different to the original behaviour? Just for cls_bpf?  
>>> >
>>> >For cls_bpf the callbacks used to be 100% symmetrical, i.e. destroy
>>> >would always be guaranteed if add succeeded (regardless of state of
>>> >skip_* flags).  
>>> 
>>> Hmm. It still should be symmetrical. Looking at following path:
>>> cls_bpf_destroy->
>>>__cls_bpf_delete->
>>>   cls_bpf_stop_offload->
>>>  cls_bpf_offload_cmd(tp, prog, TC_CLSBPF_DESTROY)
>>> 
>>> I don't see how any tp could be missed. Could you please check this
>>> callpath is utilized during your action (rmmod or qdisc del)?
>>
>>The same path seems to be utilized but the unbind comes before the
>>filters are destroyed.
>
>Ah, will fix. Thanks!

We need to move tcf_block_offload_unbind(block, q, ei); call after the
chains are flushed. There are big waves around this code in net and
net-next atm. Will send a patch fix this once the storm is over.

Thanks!

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jiri Pirko]

Sat, Oct 28, 2017 at 07:17:24PM CEST, kubak...@wp.pl wrote:
>On Sat, 28 Oct 2017 10:43:51 +0200, Jiri Pirko wrote:
>> Sat, Oct 28, 2017 at 09:53:21AM CEST, kubak...@wp.pl wrote:
>> >On Sat, 28 Oct 2017 09:20:31 +0200, Jiri Pirko wrote:  
>> >> Sat, Oct 28, 2017 at 02:52:00AM CEST, kubak...@wp.pl wrote:  
>> >> >On Fri, 27 Oct 2017 09:27:30 +0200, Jiri Pirko wrote:
>> >> >> Yes, it is the same.
>> >> >
>> >> >FWIW I also see what Amritha and Alex are describing here, for cls_bpf
>> >> >there are no DESTROYs coming on rmmod or qdisc del.  There is a DESTROY
>> >> >if I manually remove the filter (or if an ADD with skip_sw fails).
>> >> 
>> >> Is this different to the original behaviour? Just for cls_bpf?  
>> >
>> >For cls_bpf the callbacks used to be 100% symmetrical, i.e. destroy
>> >would always be guaranteed if add succeeded (regardless of state of
>> >skip_* flags).  
>> 
>> Hmm. It still should be symmetrical. Looking at following path:
>> cls_bpf_destroy->
>>__cls_bpf_delete->
>>   cls_bpf_stop_offload->
>>  cls_bpf_offload_cmd(tp, prog, TC_CLSBPF_DESTROY)
>> 
>> I don't see how any tp could be missed. Could you please check this
>> callpath is utilized during your action (rmmod or qdisc del)?
>
>The same path seems to be utilized but the unbind comes before the
>filters are destroyed.

Ah, will fix. Thanks!

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jakub Kicinski]

On Sat, 28 Oct 2017 10:43:51 +0200, Jiri Pirko wrote:
> Sat, Oct 28, 2017 at 09:53:21AM CEST, kubak...@wp.pl wrote:
> >On Sat, 28 Oct 2017 09:20:31 +0200, Jiri Pirko wrote:  
> >> Sat, Oct 28, 2017 at 02:52:00AM CEST, kubak...@wp.pl wrote:  
> >> >On Fri, 27 Oct 2017 09:27:30 +0200, Jiri Pirko wrote:
> >> >> Yes, it is the same.
> >> >
> >> >FWIW I also see what Amritha and Alex are describing here, for cls_bpf
> >> >there are no DESTROYs coming on rmmod or qdisc del.  There is a DESTROY
> >> >if I manually remove the filter (or if an ADD with skip_sw fails).
> >> 
> >> Is this different to the original behaviour? Just for cls_bpf?  
> >
> >For cls_bpf the callbacks used to be 100% symmetrical, i.e. destroy
> >would always be guaranteed if add succeeded (regardless of state of
> >skip_* flags).  
> 
> Hmm. It still should be symmetrical. Looking at following path:
> cls_bpf_destroy->
>__cls_bpf_delete->
>   cls_bpf_stop_offload->
>  cls_bpf_offload_cmd(tp, prog, TC_CLSBPF_DESTROY)
> 
> I don't see how any tp could be missed. Could you please check this
> callpath is utilized during your action (rmmod or qdisc del)?

The same path seems to be utilized but the unbind comes before the
filters are destroyed.

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jiri Pirko]

Sat, Oct 28, 2017 at 09:53:21AM CEST, kubak...@wp.pl wrote:
>On Sat, 28 Oct 2017 09:20:31 +0200, Jiri Pirko wrote:
>> Sat, Oct 28, 2017 at 02:52:00AM CEST, kubak...@wp.pl wrote:
>> >On Fri, 27 Oct 2017 09:27:30 +0200, Jiri Pirko wrote:  
>> >> Yes, it is the same.  
>> >
>> >FWIW I also see what Amritha and Alex are describing here, for cls_bpf
>> >there are no DESTROYs coming on rmmod or qdisc del.  There is a DESTROY
>> >if I manually remove the filter (or if an ADD with skip_sw fails).  
>> 
>> Is this different to the original behaviour? Just for cls_bpf?
>
>For cls_bpf the callbacks used to be 100% symmetrical, i.e. destroy
>would always be guaranteed if add succeeded (regardless of state of
>skip_* flags).

Hmm. It still should be symmetrical. Looking at following path:
cls_bpf_destroy->
   __cls_bpf_delete->
  cls_bpf_stop_offload->
 cls_bpf_offload_cmd(tp, prog, TC_CLSBPF_DESTROY)

I don't see how any tp could be missed. Could you please check this
callpath is utilized during your action (rmmod or qdisc del)?


>
>I haven't checked cls_flower on the nfp because it implodes on add
>already...  I hear the fix is simple so I can check, if that helps.
>Although from quick code inspection it seems fl_hw_destroy_filter()
>was always invoked before fl_destroy_filter(), so it looks like since
>flower doesn't track which filters were offloaded successfully it may
>send destroy events for filter drivers don't hold, but destroy should
>always be guaranteed there too.

You are right, that is following path:
fl_delete->
   fl_hw_destroy_filter

I will check it.

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jakub Kicinski]

On Sat, 28 Oct 2017 09:20:31 +0200, Jiri Pirko wrote:
> Sat, Oct 28, 2017 at 02:52:00AM CEST, kubak...@wp.pl wrote:
> >On Fri, 27 Oct 2017 09:27:30 +0200, Jiri Pirko wrote:  
> >> Yes, it is the same.  
> >
> >FWIW I also see what Amritha and Alex are describing here, for cls_bpf
> >there are no DESTROYs coming on rmmod or qdisc del.  There is a DESTROY
> >if I manually remove the filter (or if an ADD with skip_sw fails).  
> 
> Is this different to the original behaviour? Just for cls_bpf?

For cls_bpf the callbacks used to be 100% symmetrical, i.e. destroy
would always be guaranteed if add succeeded (regardless of state of
skip_* flags).

I haven't checked cls_flower on the nfp because it implodes on add
already...  I hear the fix is simple so I can check, if that helps.
Although from quick code inspection it seems fl_hw_destroy_filter()
was always invoked before fl_destroy_filter(), so it looks like since
flower doesn't track which filters were offloaded successfully it may
send destroy events for filter drivers don't hold, but destroy should
always be guaranteed there too.

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jiri Pirko]

Sat, Oct 28, 2017 at 02:52:00AM CEST, kubak...@wp.pl wrote:
>On Fri, 27 Oct 2017 09:27:30 +0200, Jiri Pirko wrote:
>> >>> 2.   Deleting the ingress qdisc fails to remove filters added in
>> >>> HW. Filters in SW gets deleted.
>> >>>
>> >>> We haven’t exactly root-caused this, the changes being extensive, but
>> >>> our guess is again something wrong with the offload check or similar
>> >>> while unregistering the block callback (tcf_block_cb_unregister) and
>> >>> further to the classifier (CLS_U32/CLS_FLOWER etc.) with the
>> >>> DESTROY/REMOVE command.  
>> >> 
>> >> Hmm. How does this worked previously. I mean, do you see change of
>> >> behaviour? I'm asking because I don't see how rules added only to HW
>> >> could be removed, driver should care of it. Or are you talking about
>> >> rules added to both SW and HW?  
>> >
>> >These are rules added to both SW and HW. Previously all cls_* had
>> >ndo_setup_tc calls based on the offload capability.
>> >
>> >commit 8d26d5636d "net: sched: avoid ndo_setup_tc calls for
>> >TC_SETUP_CLS*" removed this bit to work with the new block callback. Is
>> >there something similar in the block callback flow while acting on the
>> >tcf_proto destroy call initiated when the qdisc is cleared?  
>> 
>> Yes, it is the same.
>
>FWIW I also see what Amritha and Alex are describing here, for cls_bpf
>there are no DESTROYs coming on rmmod or qdisc del.  There is a DESTROY
>if I manually remove the filter (or if an ADD with skip_sw fails).

Is this different to the original behaviour? Just for cls_bpf?

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jakub Kicinski]

On Fri, 27 Oct 2017 09:27:30 +0200, Jiri Pirko wrote:
> >>> 2.   Deleting the ingress qdisc fails to remove filters added in
> >>> HW. Filters in SW gets deleted.
> >>>
> >>> We haven’t exactly root-caused this, the changes being extensive, but
> >>> our guess is again something wrong with the offload check or similar
> >>> while unregistering the block callback (tcf_block_cb_unregister) and
> >>> further to the classifier (CLS_U32/CLS_FLOWER etc.) with the
> >>> DESTROY/REMOVE command.  
> >> 
> >> Hmm. How does this worked previously. I mean, do you see change of
> >> behaviour? I'm asking because I don't see how rules added only to HW
> >> could be removed, driver should care of it. Or are you talking about
> >> rules added to both SW and HW?  
> >
> >These are rules added to both SW and HW. Previously all cls_* had
> >ndo_setup_tc calls based on the offload capability.
> >
> >commit 8d26d5636d "net: sched: avoid ndo_setup_tc calls for
> >TC_SETUP_CLS*" removed this bit to work with the new block callback. Is
> >there something similar in the block callback flow while acting on the
> >tcf_proto destroy call initiated when the qdisc is cleared?  
> 
> Yes, it is the same.

FWIW I also see what Amritha and Alex are describing here, for cls_bpf
there are no DESTROYs coming on rmmod or qdisc del.  There is a DESTROY
if I manually remove the filter (or if an ADD with skip_sw fails).

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jiri Pirko]

Thu, Oct 26, 2017 at 10:24:31PM CEST, amritha.namb...@intel.com wrote:
>On 10/25/2017 5:15 AM, Jiri Pirko wrote:
>> Tue, Oct 24, 2017 at 06:01:34PM CEST, alexander.du...@gmail.com wrote:
>> 
>> [...]
>> 
>>> 1.   To offload filter into HW, the hw-tc-offload feature flag has
>>> to be turned on before creating the ingress qdisc.
>>>
>>> Previously, this could also be turned on after the qdisc was created
>>> and the filters could still be offloaded. Looks like this is because,
>>> previously the offload flag was checked as a part of filter
>>> integration in the classifier, and now it is checked as part of qdisc
>>> creation (ingress_init). So, if no offload capability is advertised at
>>> ingress qdisc creation time then hardware will not be asked to offload
>>> filters later if the flag is enabled.
>> 
>> I have patchset that fixes this in my queue now. Will do some smoke
>> testing and send later today.
>> 
>> 
>>>
>>> 2.   Deleting the ingress qdisc fails to remove filters added in
>>> HW. Filters in SW gets deleted.
>>>
>>> We haven’t exactly root-caused this, the changes being extensive, but
>>> our guess is again something wrong with the offload check or similar
>>> while unregistering the block callback (tcf_block_cb_unregister) and
>>> further to the classifier (CLS_U32/CLS_FLOWER etc.) with the
>>> DESTROY/REMOVE command.
>> 
>> Hmm. How does this worked previously. I mean, do you see change of
>> behaviour? I'm asking because I don't see how rules added only to HW
>> could be removed, driver should care of it. Or are you talking about
>> rules added to both SW and HW?
>
>These are rules added to both SW and HW. Previously all cls_* had
>ndo_setup_tc calls based on the offload capability.
>
>commit 8d26d5636d "net: sched: avoid ndo_setup_tc calls for
>TC_SETUP_CLS*" removed this bit to work with the new block callback. Is
>there something similar in the block callback flow while acting on the
>tcf_proto destroy call initiated when the qdisc is cleared?

Yes, it is the same.

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Nambiar, Amritha]

On 10/25/2017 5:15 AM, Jiri Pirko wrote:
> Tue, Oct 24, 2017 at 06:01:34PM CEST, alexander.du...@gmail.com wrote:
> 
> [...]
> 
>> 1.   To offload filter into HW, the hw-tc-offload feature flag has
>> to be turned on before creating the ingress qdisc.
>>
>> Previously, this could also be turned on after the qdisc was created
>> and the filters could still be offloaded. Looks like this is because,
>> previously the offload flag was checked as a part of filter
>> integration in the classifier, and now it is checked as part of qdisc
>> creation (ingress_init). So, if no offload capability is advertised at
>> ingress qdisc creation time then hardware will not be asked to offload
>> filters later if the flag is enabled.
> 
> I have patchset that fixes this in my queue now. Will do some smoke
> testing and send later today.
> 
> 
>>
>> 2.   Deleting the ingress qdisc fails to remove filters added in
>> HW. Filters in SW gets deleted.
>>
>> We haven’t exactly root-caused this, the changes being extensive, but
>> our guess is again something wrong with the offload check or similar
>> while unregistering the block callback (tcf_block_cb_unregister) and
>> further to the classifier (CLS_U32/CLS_FLOWER etc.) with the
>> DESTROY/REMOVE command.
> 
> Hmm. How does this worked previously. I mean, do you see change of
> behaviour? I'm asking because I don't see how rules added only to HW
> could be removed, driver should care of it. Or are you talking about
> rules added to both SW and HW?

These are rules added to both SW and HW. Previously all cls_* had
ndo_setup_tc calls based on the offload capability.

commit 8d26d5636d "net: sched: avoid ndo_setup_tc calls for
TC_SETUP_CLS*" removed this bit to work with the new block callback. Is
there something similar in the block callback flow while acting on the
tcf_proto destroy call initiated when the qdisc is cleared?

> 
> Thanks!
> 

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Or Gerlitz]

On 10/25/2017 4:42 PM, Jiri Pirko wrote:

Yes, that is intentional. The thing is, there might be multiple block
callbacks registered and to be called. If there is a fail with one, we
need to cleanup all. So in your case you have 1 cb registered, that
means that in case of an error during insertion, you will get cb called
to remove. Driver has to take care of that. I was checking that and was
under impression that mlx5 deals with that.


I see, yeah, in mlx5 we identify that the flow doesn't exist in our 
tables, I sent a patch that deals with that and also another issue, lets 
discuss it there.

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jiri Pirko]

Wed, Oct 25, 2017 at 03:29:27PM CEST, ogerl...@mellanox.com wrote:
>On 10/25/2017 3:15 PM, Jiri Pirko wrote:
>> > 2. Deleting the ingress qdisc fails to remove filters added in
>> > HW. Filters in SW gets deleted.
>> > 
>> > We haven’t exactly root-caused this, the changes being extensive, but our 
>> > guess is again something wrong with the offload check or similar while 
>> > unregistering the block callback (tcf_block_cb_unregister) and further to 
>> > the classifier (CLS_U32/CLS_FLOWER etc.) with the DESTROY/REMOVE command.
>> Hmm. How does this worked previously. I mean, do you see change of
>> behaviour? I'm asking because I don't see how rules added only to HW
>> could be removed, driver should care of it. Or are you talking about
>> rules added to both SW and HW?
>
>Jiri, on a possibly related note, dealing with some other tc/flower problems

Unrelated. What you describe is a separate issue.


>on net, I came across a situation where we fail in the driverto offload some
>flow (return -EINVALtowards the stack), and we immediately get a call from
>the stack to delete this flow (f->cookie)
>
>this is the cookie and thereturn value
>
>mlx5e_configure_flower f->cookie c50e8c80 err -22
>
>and then we getthis cookie for deletion where we fail again, b/c the flow is
>not offloaded
>
>mlx5e_delete_flower f->cookie c50e8c80

Yes, that is intentional. The thing is, there might be multiple block
callbacks registered and to be called. If there is a fail with one, we
need to cleanup all. So in your case you have 1 cb registered, that
means that in case of an error during insertion, you will get cb called
to remove. Driver has to take care of that. I was checking that and was
under impression that mlx5 deals with that.

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jiri Pirko]

Tue, Oct 24, 2017 at 06:01:34PM CEST, alexander.du...@gmail.com wrote:

[...]

>1.   To offload filter into HW, the hw-tc-offload feature flag has
>to be turned on before creating the ingress qdisc.
>
>Previously, this could also be turned on after the qdisc was created
>and the filters could still be offloaded. Looks like this is because,
>previously the offload flag was checked as a part of filter
>integration in the classifier, and now it is checked as part of qdisc
>creation (ingress_init). So, if no offload capability is advertised at
>ingress qdisc creation time then hardware will not be asked to offload
>filters later if the flag is enabled.

I have patchset that fixes this in my queue now. Will do some smoke
testing and send later today.


>
>2.   Deleting the ingress qdisc fails to remove filters added in
>HW. Filters in SW gets deleted.
>
>We haven’t exactly root-caused this, the changes being extensive, but
>our guess is again something wrong with the offload check or similar
>while unregistering the block callback (tcf_block_cb_unregister) and
>further to the classifier (CLS_U32/CLS_FLOWER etc.) with the
>DESTROY/REMOVE command.

Hmm. How does this worked previously. I mean, do you see change of
behaviour? I'm asking because I don't see how rules added only to HW
could be removed, driver should care of it. Or are you talking about
rules added to both SW and HW?

Thanks!

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jiri Pirko]

Tue, Oct 24, 2017 at 06:24:01PM CEST, alexander.du...@gmail.com wrote:
>On Tue, Oct 24, 2017 at 9:01 AM, Alexander Duyck
> wrote:
>> On Fri, Oct 20, 2017 at 7:04 PM, David Miller  wrote:
>>> From: Jiri Pirko 
>>> Date: Thu, 19 Oct 2017 15:50:28 +0200
>>>
 This patchset is a bit bigger, but most of the patches are doing the
 same changes in multiple classifiers and drivers. I could do some
 squashes, but I think it is better split.

 This is another dependency on the way to shared block implementation.
 The goal is to remove use of tp->q in classifiers code.

 Also, this provides drivers possibility to track binding of blocks to
 qdiscs. Legacy drivers which do not support shared block offloading.
 register one callback per binding. That maintains the current
 functionality we have with ndo_setup_tc. Drivers which support block
 sharing offload register one callback per block which safes overhead.

 Patches 1-4 introduce the binding notifications and per-block callbacks
 Patches 5-8 add block callbacks calls to classifiers
 Patches 9-17 do convert from ndo_setup_tc calls to block callbacks for
  classifier offloads in drivers
 Patches 18-20 do cleanup
>>>
>>> Series applied.
>>
>> We are getting internal reports of regressions being seen with this
>> patch set applied. Specifically the issues below have been pointed out
>> to me. My understanding is that these issues are all being reported on
>> ixgbe:
>>
>> 1.   To offload filter into HW, the hw-tc-offload feature flag has
>> to be turned on before creating the ingress qdisc.
>>
>> Previously, this could also be turned on after the qdisc was created
>> and the filters could still be offloaded. Looks like this is because,
>> previously the offload flag was checked as a part of filter
>> integration in the classifier, and now it is checked as part of qdisc
>> creation (ingress_init). So, if no offload capability is advertised at
>> ingress qdisc creation time then hardware will not be asked to offload
>> filters later if the flag is enabled.
>>
>> 2.   Deleting the ingress qdisc fails to remove filters added in
>> HW. Filters in SW gets deleted.
>>
>> We haven’t exactly root-caused this, the changes being extensive, but
>> our guess is again something wrong with the offload check or similar
>> while unregistering the block callback (tcf_block_cb_unregister) and
>> further to the classifier (CLS_U32/CLS_FLOWER etc.) with the
>> DESTROY/REMOVE command.
>>
>> 3.   Deleting u32 filters using handle fails to remove filter from
>> HW, filter in SW gets deleted.
>>
>> Probably similar reasons, also we see some u32 specific patches as
>> well for remove nodes.
>>
>> We are still digging into this further, but wanted to put this out
>> there so we can address the issues before we go much further down this
>> path.
>>
>> Thanks.
>>
>> - Alex
>
>So a quick update. Item 3 is no longer an issue, it was a
>configuration issue on our side. So only items 1 and 2 are left to be
>addressed.

Will look at 1 and 2. Thanks for the report.

>
>Thanks.
>
>- Alex

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Nambiar, Amritha]

On 10/24/2017 10:02 AM, Or Gerlitz wrote:
> On Tue, Oct 24, 2017 at 7:24 PM, Alexander Duyck
> > wrote:
> 
> On Tue, Oct 24, 2017 at 9:01 AM, Alexander Duyck
> > wrote:
> 
>  
> 
> > We are getting internal reports of regressions being seen with this
> > patch set applied. Specifically the issues below have been pointed out
> > to me. My understanding is that these issues are all being reported on
> > ixgbe:
> >
> > 1.       To offload filter into HW, the hw-tc-offload feature flag has
> > to be turned on before creating the ingress qdisc.
> 
>  
> 
> > 2.       Deleting the ingress qdisc fails to remove filters added in
> > HW. Filters in SW gets deleted.
> 
> 
> 
> FWIW, I don't think this is what you're hitting, but please note there
> are two  fixes 
> to the series in net-next 
> 
> 9d452ce net/sched: Fix actions list corruption when adding offloaded tc
> flows

It looks like this patch just got applied, this fix wasn't included
during our testing.

> 0843c09 net/sched: Set the net-device for egress device instance

This fix was already in place during our testing. Also, the filters
added were matching on IPv4 addresses with action drop.

> I had to looked on other stuff today, but I will keep stressing see
> if/what is broken w.r.t mlx5 
> and help to get things to work later this and next week so by netdev
> we'll be fine
> 
> Or.
> 
> 

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Or Gerlitz]

On Tue, Oct 24, 2017 at 8:02 PM, Or Gerlitz  wrote:
> On Tue, Oct 24, 2017 at 7:24 PM, Alexander Duyck  
> wrote:
>> On Tue, Oct 24, 2017 at 9:01 AM, Alexander Duyck  
>> wrote:

>>> We are getting internal reports of regressions being seen with this
>>> patch set applied. Specifically the issues below have been pointed out
>>> to me. My understanding is that these issues are all being reported on 
>>> ixgbe:

>>> 1.   To offload filter into HW, the hw-tc-offload feature flag has
>>> to be turned on before creating the ingress qdisc.

>>> 2.   Deleting the ingress qdisc fails to remove filters added in
>>> HW. Filters in SW gets deleted.

> FWIW, I don't think this is what you're hitting, but please note there are
> two  fixes  to the series in net-next
>
> 9d452ce net/sched: Fix actions list corruption when adding offloaded tc flows
> 0843c09 net/sched: Set the net-device for egress device instance
>
> I had to looked on other stuff today, but I will keep stressing see if/what
> is broken w.r.t mlx5
> and help to get things to work later this and next week so by netdev we'll
> be fine
>
> Or.

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Alexander Duyck]

On Tue, Oct 24, 2017 at 9:01 AM, Alexander Duyck
 wrote:
> On Fri, Oct 20, 2017 at 7:04 PM, David Miller  wrote:
>> From: Jiri Pirko 
>> Date: Thu, 19 Oct 2017 15:50:28 +0200
>>
>>> This patchset is a bit bigger, but most of the patches are doing the
>>> same changes in multiple classifiers and drivers. I could do some
>>> squashes, but I think it is better split.
>>>
>>> This is another dependency on the way to shared block implementation.
>>> The goal is to remove use of tp->q in classifiers code.
>>>
>>> Also, this provides drivers possibility to track binding of blocks to
>>> qdiscs. Legacy drivers which do not support shared block offloading.
>>> register one callback per binding. That maintains the current
>>> functionality we have with ndo_setup_tc. Drivers which support block
>>> sharing offload register one callback per block which safes overhead.
>>>
>>> Patches 1-4 introduce the binding notifications and per-block callbacks
>>> Patches 5-8 add block callbacks calls to classifiers
>>> Patches 9-17 do convert from ndo_setup_tc calls to block callbacks for
>>>  classifier offloads in drivers
>>> Patches 18-20 do cleanup
>>
>> Series applied.
>
> We are getting internal reports of regressions being seen with this
> patch set applied. Specifically the issues below have been pointed out
> to me. My understanding is that these issues are all being reported on
> ixgbe:
>
> 1.   To offload filter into HW, the hw-tc-offload feature flag has
> to be turned on before creating the ingress qdisc.
>
> Previously, this could also be turned on after the qdisc was created
> and the filters could still be offloaded. Looks like this is because,
> previously the offload flag was checked as a part of filter
> integration in the classifier, and now it is checked as part of qdisc
> creation (ingress_init). So, if no offload capability is advertised at
> ingress qdisc creation time then hardware will not be asked to offload
> filters later if the flag is enabled.
>
> 2.   Deleting the ingress qdisc fails to remove filters added in
> HW. Filters in SW gets deleted.
>
> We haven’t exactly root-caused this, the changes being extensive, but
> our guess is again something wrong with the offload check or similar
> while unregistering the block callback (tcf_block_cb_unregister) and
> further to the classifier (CLS_U32/CLS_FLOWER etc.) with the
> DESTROY/REMOVE command.
>
> 3.   Deleting u32 filters using handle fails to remove filter from
> HW, filter in SW gets deleted.
>
> Probably similar reasons, also we see some u32 specific patches as
> well for remove nodes.
>
> We are still digging into this further, but wanted to put this out
> there so we can address the issues before we go much further down this
> path.
>
> Thanks.
>
> - Alex

So a quick update. Item 3 is no longer an issue, it was a
configuration issue on our side. So only items 1 and 2 are left to be
addressed.

Thanks.

- Alex

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Alexander Duyck]

On Fri, Oct 20, 2017 at 7:04 PM, David Miller  wrote:
> From: Jiri Pirko 
> Date: Thu, 19 Oct 2017 15:50:28 +0200
>
>> This patchset is a bit bigger, but most of the patches are doing the
>> same changes in multiple classifiers and drivers. I could do some
>> squashes, but I think it is better split.
>>
>> This is another dependency on the way to shared block implementation.
>> The goal is to remove use of tp->q in classifiers code.
>>
>> Also, this provides drivers possibility to track binding of blocks to
>> qdiscs. Legacy drivers which do not support shared block offloading.
>> register one callback per binding. That maintains the current
>> functionality we have with ndo_setup_tc. Drivers which support block
>> sharing offload register one callback per block which safes overhead.
>>
>> Patches 1-4 introduce the binding notifications and per-block callbacks
>> Patches 5-8 add block callbacks calls to classifiers
>> Patches 9-17 do convert from ndo_setup_tc calls to block callbacks for
>>  classifier offloads in drivers
>> Patches 18-20 do cleanup
>
> Series applied.

We are getting internal reports of regressions being seen with this
patch set applied. Specifically the issues below have been pointed out
to me. My understanding is that these issues are all being reported on
ixgbe:

1.   To offload filter into HW, the hw-tc-offload feature flag has
to be turned on before creating the ingress qdisc.

Previously, this could also be turned on after the qdisc was created
and the filters could still be offloaded. Looks like this is because,
previously the offload flag was checked as a part of filter
integration in the classifier, and now it is checked as part of qdisc
creation (ingress_init). So, if no offload capability is advertised at
ingress qdisc creation time then hardware will not be asked to offload
filters later if the flag is enabled.

2.   Deleting the ingress qdisc fails to remove filters added in
HW. Filters in SW gets deleted.

We haven’t exactly root-caused this, the changes being extensive, but
our guess is again something wrong with the offload check or similar
while unregistering the block callback (tcf_block_cb_unregister) and
further to the classifier (CLS_U32/CLS_FLOWER etc.) with the
DESTROY/REMOVE command.

3.   Deleting u32 filters using handle fails to remove filter from
HW, filter in SW gets deleted.

Probably similar reasons, also we see some u32 specific patches as
well for remove nodes.

We are still digging into this further, but wanted to put this out
there so we can address the issues before we go much further down this
path.

Thanks.

- Alex

Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[David Miller]

From: Jiri Pirko 
Date: Thu, 19 Oct 2017 15:50:28 +0200

> This patchset is a bit bigger, but most of the patches are doing the
> same changes in multiple classifiers and drivers. I could do some
> squashes, but I think it is better split.
> 
> This is another dependency on the way to shared block implementation.
> The goal is to remove use of tp->q in classifiers code.
> 
> Also, this provides drivers possibility to track binding of blocks to
> qdiscs. Legacy drivers which do not support shared block offloading.
> register one callback per binding. That maintains the current
> functionality we have with ndo_setup_tc. Drivers which support block
> sharing offload register one callback per block which safes overhead.
> 
> Patches 1-4 introduce the binding notifications and per-block callbacks
> Patches 5-8 add block callbacks calls to classifiers
> Patches 9-17 do convert from ndo_setup_tc calls to block callbacks for
>  classifier offloads in drivers
> Patches 18-20 do cleanup

Series applied.

[patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks

[Jiri Pirko]

From: Jiri Pirko 

This patchset is a bit bigger, but most of the patches are doing the
same changes in multiple classifiers and drivers. I could do some
squashes, but I think it is better split.

This is another dependency on the way to shared block implementation.
The goal is to remove use of tp->q in classifiers code.

Also, this provides drivers possibility to track binding of blocks to
qdiscs. Legacy drivers which do not support shared block offloading.
register one callback per binding. That maintains the current
functionality we have with ndo_setup_tc. Drivers which support block
sharing offload register one callback per block which safes overhead.

Patches 1-4 introduce the binding notifications and per-block callbacks
Patches 5-8 add block callbacks calls to classifiers
Patches 9-17 do convert from ndo_setup_tc calls to block callbacks for
 classifier offloads in drivers
Patches 18-20 do cleanup

---
v1->v2:
- patch1:
  - move new enum value to the end

Jiri Pirko (20):
  net: sched: add block bind/unbind notif. and extended block_get/put
  net: sched: use extended variants of block_get/put in ingress and
clsact qdiscs
  net: sched: introduce per-block callbacks
  net: sched: use tc_setup_cb_call to call per-block callbacks
  net: sched: cls_matchall: call block callbacks for offload
  net: sched: cls_u32: swap u32_remove_hw_knode and u32_remove_hw_hnode
  net: sched: cls_u32: call block callbacks for offload
  net: sched: cls_bpf: call block callbacks for offload
  mlxsw: spectrum: Convert ndo_setup_tc offloads to block callbacks
  mlx5e: Convert ndo_setup_tc offloads to block callbacks
  bnxt: Convert ndo_setup_tc offloads to block callbacks
  cxgb4: Convert ndo_setup_tc offloads to block callbacks
  ixgbe: Convert ndo_setup_tc offloads to block callbacks
  mlx5e_rep: Convert ndo_setup_tc offloads to block callbacks
  nfp: flower: Convert ndo_setup_tc offloads to block callbacks
  nfp: bpf: Convert ndo_setup_tc offloads to block callbacks
  dsa: Convert ndo_setup_tc offloads to block callbacks
  net: sched: avoid ndo_setup_tc calls for TC_SETUP_CLS*
  net: sched: remove unused classid field from tc_cls_common_offload
  net: sched: remove unused is_classid_clsact_ingress/egress helpers

 drivers/net/ethernet/broadcom/bnxt/bnxt.c  |  37 -
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c   |   3 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c  |  41 -
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c|  42 -
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  45 -
 drivers/net/ethernet/mellanox/mlx5/core/en.h   |   4 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c  |  45 -
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c   |  62 +--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c |  83 +++---
 drivers/net/ethernet/netronome/nfp/bpf/main.c  |  52 +-
 .../net/ethernet/netronome/nfp/flower/offload.c|  54 +-
 include/linux/netdevice.h  |   1 +
 include/net/pkt_cls.h  | 129 ++-
 include/net/pkt_sched.h|  13 --
 include/net/sch_generic.h  |   1 +
 net/dsa/slave.c|  64 ++--
 net/sched/cls_api.c| 182 -
 net/sched/cls_bpf.c|  28 +++-
 net/sched/cls_flower.c |  29 +---
 net/sched/cls_matchall.c   |  58 +++
 net/sched/cls_u32.c|  67 
 net/sched/sch_ingress.c|  36 +++-
 22 files changed, 849 insertions(+), 227 deletions(-)

-- 
2.9.5