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.
>
> 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().
> 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
