On 2023/05/22 20:41, Ilya Maximets wrote:
> On 5/22/23 07:05, Nobuhiro MIKI wrote:
>> On 2023/05/20 10:12, Ilya Maximets wrote:
>>> On 5/20/23 02:34, Ilya Maximets wrote:
>>>> On 5/16/23 07:33, Nobuhiro MIKI wrote:
>>>>> It supports flowlabel based load balancing by controlling the flowlabel
>>>>> of outer IPv6 header, which is already implemented in Linux kernel as
>>>>> seg6_flowlabel sysctl [1].
>>>>>
>>>>> [1]: https://docs.kernel.org/networking/seg6-sysctl.html
>>>>>
>>>>> Signed-off-by: Nobuhiro MIKI <[email protected]>
>>>>> ---
>>>>> include/linux/openvswitch.h | 3 +-
>>>>> lib/netdev-native-tnl.c | 23 ++++++++++-
>>>>> lib/netdev-vport.c | 8 ++++
>>>>> lib/netdev.h | 12 ++++++
>>>>> tests/tunnel-push-pop-ipv6.at | 72 +++++++++++++++++++++++++++++++++++
>>>>> vswitchd/vswitch.xml | 26 +++++++++++++
>>>>> 6 files changed, 141 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>>>> index e305c331516b..278829cfa826 100644
>>>>> --- a/include/linux/openvswitch.h
>>>>> +++ b/include/linux/openvswitch.h
>>>>> @@ -855,7 +855,8 @@ struct ovs_action_push_tnl {
>>>>> odp_port_t tnl_port;
>>>>> odp_port_t out_port;
>>>>> uint32_t header_len;
>>>>> - uint32_t tnl_type; /* For logging. */
>>>>> + uint32_t tnl_type; /* For logging. */
>>>>> + uint32_t srv6_flowlabel; /* Only for SRv6. */
>>>>> uint32_t header[TNL_PUSH_HEADER_SIZE / 4];
>>>>> };
>>>>> #endif
>>>>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
>>>>> index db1c4c6d9bfc..796169fe43ac 100644
>>>>> --- a/lib/netdev-native-tnl.c
>>>>> +++ b/lib/netdev-native-tnl.c
>>>>> @@ -910,6 +910,7 @@ netdev_srv6_build_header(const struct netdev *netdev,
>>>>> }
>>>>>
>>>>> data->header_len += sizeof *srh + 8 * srh->rt_hdr.hdrlen;
>>>>> + data->srv6_flowlabel = tnl_cfg->srv6_flowlabel;
>>>>> data->tnl_type = OVS_VPORT_TYPE_SRV6;
>>>>> out:
>>>>> ovs_mutex_unlock(&dev->mutex);
>>>>> @@ -922,10 +923,28 @@ netdev_srv6_push_header(const struct netdev *netdev
>>>>> OVS_UNUSED,
>>>>> struct dp_packet *packet,
>>>>> const struct ovs_action_push_tnl *data)
>>>>> {
>>>>> + struct ovs_16aligned_ip6_hdr *ip6 = dp_packet_l3(packet);
>>>>> + ovs_be32 ipv6_label = 0;
>>>>> int ip_tot_size;
>>>>> + uint32_t flow;
>>>>>
>>>>> - netdev_tnl_push_ip_header(packet, data->header, data->header_len,
>>>>> - &ip_tot_size, 0);
>>>>> + switch (data->srv6_flowlabel) {
>>>>
>>>> I understand why you added a new filed into the ovs_action_push_tnl
>>>> structure,
>>>> but I'm not sure we should do that.
>>>>
>>>> If you'll look at the GRE seqno, for exmaple, you'll see that we're
>>>> accessing
>>>> it via the original tnl_cfg structure stored in netdev. The problem is
>>>> that
>>>> it's not really thread safe, as I pointed out in review for the previous
>>>> version.
>>>> I went ahead and tried to fix the thread-safety issues. Patch set posted
>>>> here:
>>>>
>>>>
>>>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=355820&state=*
>>>>
>>>> After these changes you should be able to directly use tunnel config like
>>>> this:
>>>>
>>>> switch (netdev_get_tunnel_config(netdev)->srv6_flowlabel) {
>>>>
>>>> And it's going to be safe to do so. And we'll not need to change the
>>>> action
>>>> structure.
>>>>
>>>> What do you think?
>>>
>>> One other option is to use the ipv6 label of the built header itself to
>>> communicate
>>> the desired mode. i.e. Write a enum value to the ipv6_label field during
>>> the header
>>> build and check it here.
>>>
>>
>> Hi Ilya,
>>
>> Thanks for your reviews.
>>
>> There is something I don't understand in the handling of tnl_cfg structure.
>> It seems that the netdev given for each of the header push and header build
>> is different.
>> For header build, t2 (which matches the port name used by ovs-ofctl) is
>> given as netdev
>> and srv6_flowlabel enum value contains a valid value. But during header
>> push, srv6_sys
>> is given as netdev and the srv6_flowlabel enum value is always 0. As for GRE
>> seqno, it
>> may not be affected by this since its initial value is 0 and it is only
>> incremented.
>
> Hmm, interesting. Thanks for pointing out! I'll take a look.
>
Thanks for your cooperation.
Please let me know if there is anything I can do to help.
>>
>> It seems to me that investigating and fixing why netdev is different for
>> header build and
>> header push would be well beyond the scope of this patch set. So, as you
>> commented, I'll
>> implement writing an enum value to the ipv6_label field at header build and
>> replacing it
>> with a hash value at header push.
>
> Ack. Sounds good to me. We also do the same thing for UDP checksum.
> We write 0xffff to the checksum field as an indicator that it needs
> to be calculated on push, see udp_build_header().
>
Thanks for giving me the example.
OK. It seems that the plan for the implementation has been decided, I'll
prepare v5.
Best Regards,
Nobuhiro MIKI
>> For reference, the investigation I did can be
>> reproduced below:
>
> Thanks.
>
>>
>> 2023-05-22T03:34:18.844Z|00157|native_tnl|INFO|netdev name in header build:
>> t2
>> 2023-05-22T03:34:18.844Z|00158|native_tnl|INFO|srv6_flowlabel in header
>> build: 1
>> 2023-05-22T03:34:18.844Z|00159|native_tnl|INFO|netdev name in header push:
>> srv6_sys
>> 2023-05-22T03:34:18.844Z|00160|native_tnl|INFO|srv6_flowlabel in header
>> push: 0
>>
>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
>> index cf7ed080a839..e941db13c46a 100644
>> --- a/lib/netdev-native-tnl.c
>> +++ b/lib/netdev-native-tnl.c
>> @@ -855,6 +855,9 @@ netdev_srv6_build_header(const struct netdev *netdev,
>> struct ovs_action_push_tnl *data,
>> const struct netdev_tnl_build_header_params
>> *params)
>> {
>> + VLOG_INFO("netdev name in header build: %s", netdev->name);
>> + VLOG_INFO("srv6_flowlabel in header build: %d",
>> netdev_get_tunnel_config(netdev)->srv6_flowlabel);
>> +
>> struct netdev_vport *dev = netdev_vport_cast(netdev);
>> struct netdev_tunnel_config *tnl_cfg;
>> const struct in6_addr *segs;
>> @@ -930,6 +933,9 @@ netdev_srv6_push_header(const struct netdev *netdev
>> OVS_UNUSED,
>> int ip_tot_size;
>> uint32_t flow;
>>
>> + VLOG_INFO("netdev name in header push: %s", netdev->name);
>> + VLOG_INFO("srv6_flowlabel in header push: %d",
>> netdev_get_tunnel_config(netdev)->srv6_flowlabel);
>> +
>> switch (data->srv6_flowlabel) {
>> case SRV6_FLOWLABEL_COPY:
>> flow = ntohl(get_16aligned_be32(&ip6->ip6_flow));
>>
>>
>> Best Regards,
>> Nobuhiro MIKI
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
