On 12/3/21 16:57, Paolo Valerio wrote:
> Ilya Maximets <[email protected]> writes:
>
>> On 11/11/21 19:06, Paolo Valerio wrote:
>>> Hi Chris,
>>>
>>> Chris Mi via dev <[email protected]> writes:
>>>
>>>> OVS_DP_F_UNALIGNED is already set, no need to set again. If restarting ovs,
>>>> dp is already created. So dpif_netlink_dp_transact() will return EEXIST.
>>>> No need to probe again.
>>>>
>>>> Signed-off-by: Chris Mi <[email protected]>
>>>> ---
>>>> lib/dpif-netlink.c | 3 +--
>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>>>> index 5f4b60c5a..baa8e4d2a 100644
>>>> --- a/lib/dpif-netlink.c
>>>> +++ b/lib/dpif-netlink.c
>>>> @@ -411,11 +411,10 @@ dpif_netlink_open(const struct dpif_class *class
>>>> OVS_UNUSED, const char *name,
>>>> * dispatching, we fall back to the per-vport dispatch mode.
>>>> */
>>>> dp_request.user_features &= ~OVS_DP_F_UNSUPPORTED;
>>>> - dp_request.user_features |= OVS_DP_F_UNALIGNED;
>>>> dp_request.user_features &= ~OVS_DP_F_VPORT_PIDS;
>>>> dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU;
>>>> error = dpif_netlink_dp_transact(&dp_request, &dp, &buf);
>>>> - if (error) {
>>>> + if (error == EOPNOTSUPP) {
>>>> dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU;
>>>> dp_request.user_features |= OVS_DP_F_VPORT_PIDS;
>>>> error = dpif_netlink_dp_transact(&dp_request, &dp, &buf);
>>>
>>> The patch LGTM, there's a remark about this function, though.
>>
>> Paolo, should I consider this as 'Acked-by' ?
>>
>
> yes, sorry, I must have forgotten to add it.
>
> Acked-by: Paolo Valerio <[email protected]>
Thanks, Chris and Paolo! Applied.
Bets regards, Ilya Maximets.
>
>>>
>>> Before [1] the datapath did not check what user_features were supported,
>>> for this reason [2] was needed to avoid the case in which we set
>>> OVS_DP_F_DISPATCH_UPCALL_PER_CPU on old kernels without supporting it.
>>>
>>> I wonder what happens if, in case of kernel without [1] (prior to
>>> 5.4), we try to create the datapath during a restart?
>>>
>>> My impression is that we'll keep transacting receiving EEXIST, and only
>>> after opening (without trying to create it) we set the things as we
>>> intend.
>>>
>>> This seems to be confirmed by:
>>>
>>> ovs-vswitchd 834 [000] 146.180045: probe:ovs_dp_cmd_new__return:
>>> (ffffffffc075d290 <- ffffffffacba1a5a) retval=0xffffffef
>>> ovs-vswitchd 834 [000] 146.180111: probe:ovs_dp_cmd_new__return:
>>> (ffffffffc075d290 <- ffffffffacba1a5a) retval=0xffffffef
>>> ovs-vswitchd 834 [000] 146.180212: probe:ovs_dp_cmd_new__return:
>>> (ffffffffc075d290 <- ffffffffacba1a5a) retval=0xffffffef
>>>
>>> Note that I didn't really test it against an old kernel, but I just
>>> removed the user_features validation from the kernel code.
>>>
>>> If confirmed, this is not a problem per se (there should not be a case
>>> where this can become a functional problem), it's more about knowing that
>>> there's this further chance to clean things up.
>>>
>>> [1] 95a7233c452a ("net: openvswitch: Set OvS recirc_id from tc chain index")
>>> [2] b841e3cd4a28 ("dpif-netlink: Fix feature negotiation for older
>>> kernels.")
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
