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?
> + 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
