On 21 Feb 2022, at 12:45, Ilya Maximets wrote:
> On 2/21/22 11:53, Eelco Chaudron wrote:
>>
>>
>> On 17 Feb 2022, at 16:07, Ilya Maximets wrote:
>>
>>> On 2/17/22 13:57, Ilya Maximets wrote:
>>>> On 1/31/22 11:34, Eelco Chaudron wrote:
>>>>> Verify that the returned ifindex by netdev_get_ifindex() is valid.
>>>>> This might not be the case in the ERSPAN port scenario, which can
>>>>> not be offloaded.
>>>>
>>>> Why exactly we can't get ifindex of the kernel interface?
>>>
>>> Hmm. I see that netdev_vport_tunnel_register() doesn't register
>>> the .get_ifindex callback for erspan and some other tunnel types,
>>> but I'm not sure why...
>>
>> Same here. I could not think of a reason. But did not want to break anything
>> by adding it.
>>
>>>>>
>>>>> Signed-off-by: Eelco Chaudron <[email protected]>
>>>>> ---
>>>>> lib/netdev-offload-tc.c | 9 +++++++++
>>>>> 1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>>>> index 9845e8d3f..8135441c6 100644
>>>>> --- a/lib/netdev-offload-tc.c
>>>>> +++ b/lib/netdev-offload-tc.c
>>>>> @@ -1842,6 +1842,15 @@ netdev_tc_flow_put(struct netdev *netdev, struct
>>>>> match *match,
>>>>> return ENODEV;
>>>>> }
>>>>> action->out.ifindex_out = netdev_get_ifindex(outdev);
>>>>> +
>>>>> + if (action->out.ifindex_out < 0) {
>>>>> + VLOG_DBG_RL(&rl,
>>>>> + "Can't find ifindex for output port %s,
>>>>> error %d",
>>>>> + netdev_get_name(outdev),
>>>>> action->out.ifindex_out);
>>>>> +
>>>>
>>>> We're leaking netdev reference here.
>>
>> Good catch!
>>
>>>> Shouldn't we check netdev_flow_api_equals(netdev, outdev) here instead?
>>>> This way we will be sure that flow API was successfully initialized for
>>>> the outdev and we're not mixing different offload providers.
>>
>> Decided to add it, and keep the index check also. So it will look something
>> like:
>>
>> @@ -1841,7 +1841,24 @@ netdev_tc_flow_put(struct netdev *netdev, struct
>> match *match,
>> VLOG_DBG_RL(&rl, "Can't find netdev for output port %d",
>> port);
>> return ENODEV;
>> }
>> +
>> + if (!netdev_flow_api_equals(netdev, outdev)) {
>> + VLOG_DBG_RL(&rl,
>> + "Egress port %s is using a different flow API "
>> + "provider", netdev_get_name(outdev));
>
> I'm not sure if the message is fully correct as egress port may not
> have any offload provider initialized. Maybe something like:
> "Flow API provider mismatch between ingress (%s) and egress (%s) ports"
> or
> "Egress port %s doesn't have or is using a different ..." (<-- this one
> should be phrased differently, I guess, but it's just an example).
>
> What do you think?
Took the first suggestion. I will send it as part of v3...
>> + netdev_close(outdev);
>> + return EOPNOTSUPP;
>> + }
>> +
>> action->out.ifindex_out = netdev_get_ifindex(outdev);
>> + if (action->out.ifindex_out < 0) {
>> + VLOG_DBG_RL(&rl,
>> + "Can't find ifindex for output port %s, error
>> %d",
>> + netdev_get_name(outdev),
>> action->out.ifindex_out);
>> + netdev_close(outdev);
>> + return -action->out.ifindex_out;
>> + }
>> +
>
> OK. Should work.
> I don't think that ifindex check is necessary here as initialization
> of tc offload provider requires having a valid ifindex, but I agree
> that extra check should not make any harm.
>
>>
>>>>> + return -action->out.ifindex_out;
>>>>> + }
>>>>> +
>>>>> action->out.ingress =
>>>>> is_internal_port(netdev_get_type(outdev));
>>>>> action->type = TC_ACT_OUTPUT;
>>>>> flower.action_count++;
>>>>>
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
