On Wed, Apr 18, 2018 at 3:31 PM, John Hurley <john.hur...@netronome.com> wrote:
> On Wed, Apr 18, 2018 at 8:43 AM, Or Gerlitz <gerlitz...@gmail.com> wrote:
>> On Fri, Nov 17, 2017 at 4:06 AM, Jakub Kicinski
>> <jakub.kicin...@netronome.com> wrote:
>>> From: John Hurley <john.hur...@netronome.com>
>>>
>>> Pass information to the match offload on whether or not the repr is the
>>> ingress or egress dev. Only accept tunnel matches if repr is the egress dev.
>>>
>>> This means rules such as the following are successfully offloaded:
>>> tc .. add dev vxlan0 .. enc_dst_port 4789 .. action redirect dev nfp_p0
>>>
>>> While rules such as the following are rejected:
>>> tc .. add dev nfp_p0 .. enc_dst_port 4789 .. action redirect dev vxlan0
>>
>> cool
>>
>>
>>> Also reject non tunnel flows that are offloaded to an egress dev.
>>> Non tunnel matches assume that the offload dev is the ingress port and
>>> offload a match accordingly.
>>
>> not following on the "Also" here, see below
>>
>>
>>> diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>> b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>> index a0193e0c24a0..f5d73b83dcc2 100644
>>> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>>> @@ -131,7 +131,8 @@ static bool nfp_flower_check_higher_than_mac(struct
>>> tc_cls_flower_offload *f)
>>>
>>> static int
>>> nfp_flower_calculate_key_layers(struct nfp_fl_key_ls *ret_key_ls,
>>> - struct tc_cls_flower_offload *flow)
>>> + struct tc_cls_flower_offload *flow,
>>> + bool egress)
>>> {
>>> struct flow_dissector_key_basic *mask_basic = NULL;
>>> struct flow_dissector_key_basic *key_basic = NULL;
>>> @@ -167,6 +168,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls
>>> *ret_key_ls,
>>> skb_flow_dissector_target(flow->dissector,
>>>
>>> FLOW_DISSECTOR_KEY_ENC_CONTROL,
>>> flow->key);
>>> + if (!egress)
>>> + return -EOPNOTSUPP;
>>> +
>>> if (mask_enc_ctl->addr_type != 0xffff ||
>>> enc_ctl->addr_type != FLOW_DISSECTOR_KEY_IPV4_ADDRS)
>>> return -EOPNOTSUPP;
>>> @@ -194,6 +198,9 @@ nfp_flower_calculate_key_layers(struct nfp_fl_key_ls
>>> *ret_key_ls,
>>>
>>> key_layer |= NFP_FLOWER_LAYER_VXLAN;
>>> key_size += sizeof(struct nfp_flower_vxlan);
>>> + } else if (egress) {
>>> + /* Reject non tunnel matches offloaded to egress repr. */
>>> + return -EOPNOTSUPP;
>>> }
>>
>> with these two hunks we get: egress <- IFF -> encap match, right?
>>
>> (1) we can't offload the egress way if there isn't matching on encap headers
>> (2) we can't go the matching on encap headers way if we are not egress
>>
>
> yes, this is correct.
> With the block code and egdev offload, we do not have access to the
> ingress netdev when doing an offload.
> We need to use the encap headers (especially the enc_port) to
> distinguish the type of tunnel used and, therefore, require that the
> encap matches be present before offloading.
>
>> what other cases are rejected by this logic?
>>
>
> Yes, some other cases may be rejected (like veth mentioned below).
my claim is that the veth case I mentioned below will not be rejected
if it has the matching on encap headers, and a wrong rule will be set
into hw, agree?
> However, this is better than allowing rules to be incorrectly
> offloaded (as could have happened before these changes).
> Currently, we are looking at offloading flows on other ingress devices
> such as bonds so this will require a change to the driver code here.
for the ingress side, Jiri suggested that the slave devices (uplink reps),
will be just getting all the rules set on the bond, so I am not sure what
problem you see here... for decap it will be still vxlan --> vf rep and your
egress logic will allow it.
> IMO, the cleanest solution will also require tc core changes to either
> avoid egdev offload or to have access to the ingress netdev of a rule.
>> e.g If we add a rule with SW device (veth. tap) being the ingress, and
>> HW device (vf rep)
>> being the egress while not using skip_sw (just no flags == both) we
>> get the TC stack
>> go along the egdev callback from the vf rep hw device and add an
>> (uplink --> vf rep) rule
>> which will not be rejected if there is matching on tunnel headers, it
>> will also not be rejected
>> by some driver logic as the one we discussed to identify and ignore
>> rules that are attempted to being added twice.
>>
>> Or.
