Re: [patch net-next v2 01/10] cls_bpf: move prog offload->netdev check into drivers

Mon, Nov 13, 2017 at 09:12:35AM CET, jakub.kicinski@netronome.com wrote:

On Mon, 13 Nov 2017 08:55:56 +0100, Jiri Pirko wrote:

quoted

Mon, Nov 13, 2017 at 08:17:34AM CET, jakub.kicinski@netronome.com wrote:

quoted

On Mon, 13 Nov 2017 07:25:38 +0100, Jiri Pirko wrote:  

quoted

Mon, Nov 13, 2017 at 03:14:18AM CET, jakub.kicinski@netronome.com wrote:  

quoted

On Sun, 12 Nov 2017 16:55:55 +0100, Jiri Pirko wrote:    

quoted

From: Jiri Pirko <redacted>

In order to remove tp->q usage in cls_bpf, the offload->netdev check
needs to be moved to individual drivers as only they will have access
to appropriate struct net_device.

Signed-off-by: Jiri Pirko <redacted>    

This seems not entirely correct and it adds unnecessary code.  I think    

What is not correct?  

From quick reading it looks like you will allow to install the
dev-specific filter without skip_sw flag.  You haven't fixed what  

Right. I see it now.

quoted

your previous series broke in cls_bpf offload model and now you   

What do you mean exactly?

As explained elsewhere, cls_bpf used to track what's offloaded and
issue ADD/REPLACE/DESTORY accordingly.  Now drivers need to know what
they're offloading, but they still don't.  So if you add a filter that
offload successfully and then one that doesn't, the spurious DESTORY
will kill the wrong offload.

Ah, got it.

quoted

quoted

break it even further.
 

quoted

quoted

the XDP and cls_bpf handling could be unified, making way for binding
the same program to multiple ports of the same device.  Would you mind
waiting a day for me to send corrections to BPF offload?    

Well I'm trying to get this in before net-next closes...  

Right, and I'm surprised by that.  I'd hope you'll understand my caution
here given recent history.  

Sure.

I looked through this series and I can't grasp all the details of how
things are supposed to work from the code here :(  Perhaps important
bits went in earlier and I missed them.

Starting from the most fundamental thing - if I have a shared block
full of skip_sw filters and then bind it to a device which doesn't even
have ndo_setup_tc - what prevents that from happening?

Nothing atm. I need to add some check there. Thanks.

AFACT tcf_block_offload_cmd() is returning void.