| Message ID | 20230516053336.27303-2-nmiki@yahoo-corp.jp |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support flowlabel calculation in SRv6 tunnels | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 5/16/23 07:33, Nobuhiro MIKI wrote: > For tunnels such as SRv6, some popular vendor appliances support > IPv6 flowlabel based load balancing. In preparation for OVS to > support it, this patch modifies the encapsulation to allow IPv6 > flowlabel to be configured. > > Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> > --- > lib/netdev-native-tnl.c | 23 +++++++++++++---------- > lib/netdev-native-tnl.h | 4 ++-- > lib/packets.c | 2 +- > lib/packets.h | 2 ++ > 4 files changed, 18 insertions(+), 13 deletions(-) > > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c > index 9abdf51076a8..db1c4c6d9bfc 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -146,8 +146,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, > * > * Return pointer to the L4 header added to 'packet'. */ > void * > -netdev_tnl_push_ip_header(struct dp_packet *packet, > - const void *header, int size, int *ip_tot_size) > +netdev_tnl_push_ip_header(struct dp_packet *packet, const void *header, > + int size, int *ip_tot_size, ovs_be32 ipv6_label) > { > struct eth_header *eth; > struct ip_header *ip; > @@ -166,6 +166,7 @@ netdev_tnl_push_ip_header(struct dp_packet *packet, > ip6 = netdev_tnl_ipv6_hdr(eth); > *ip_tot_size -= IPV6_HEADER_LEN; > ip6->ip6_plen = htons(*ip_tot_size); > + packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); This function is on a hot path. It might make sense to update the flow label only if it is non-zero, to save some cycles. Best regards, Ilya Maximets.
On 2023/05/20 3:56, Ilya Maximets wrote: > On 5/16/23 07:33, Nobuhiro MIKI wrote: >> For tunnels such as SRv6, some popular vendor appliances support >> IPv6 flowlabel based load balancing. In preparation for OVS to >> support it, this patch modifies the encapsulation to allow IPv6 >> flowlabel to be configured. >> >> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> >> --- >> lib/netdev-native-tnl.c | 23 +++++++++++++---------- >> lib/netdev-native-tnl.h | 4 ++-- >> lib/packets.c | 2 +- >> lib/packets.h | 2 ++ >> 4 files changed, 18 insertions(+), 13 deletions(-) >> >> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c >> index 9abdf51076a8..db1c4c6d9bfc 100644 >> --- a/lib/netdev-native-tnl.c >> +++ b/lib/netdev-native-tnl.c >> @@ -146,8 +146,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, >> * >> * Return pointer to the L4 header added to 'packet'. */ >> void * >> -netdev_tnl_push_ip_header(struct dp_packet *packet, >> - const void *header, int size, int *ip_tot_size) >> +netdev_tnl_push_ip_header(struct dp_packet *packet, const void *header, >> + int size, int *ip_tot_size, ovs_be32 ipv6_label) >> { >> struct eth_header *eth; >> struct ip_header *ip; >> @@ -166,6 +166,7 @@ netdev_tnl_push_ip_header(struct dp_packet *packet, >> ip6 = netdev_tnl_ipv6_hdr(eth); >> *ip_tot_size -= IPV6_HEADER_LEN; >> ip6->ip6_plen = htons(*ip_tot_size); >> + packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); > > This function is on a hot path. It might make sense to update > the flow label only if it is non-zero, to save some cycles. > Hi Ilya, Thanks for your review. Yes, I agree. I will simply insert an if block as follows: - packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); + if (ipv6_label) { + packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); + } Best Regards, Nobuhiro MIKI
On 2023/05/22 12:09, Nobuhiro MIKI wrote: > On 2023/05/20 3:56, Ilya Maximets wrote: >> On 5/16/23 07:33, Nobuhiro MIKI wrote: >>> For tunnels such as SRv6, some popular vendor appliances support >>> IPv6 flowlabel based load balancing. In preparation for OVS to >>> support it, this patch modifies the encapsulation to allow IPv6 >>> flowlabel to be configured. >>> >>> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> >>> --- >>> lib/netdev-native-tnl.c | 23 +++++++++++++---------- >>> lib/netdev-native-tnl.h | 4 ++-- >>> lib/packets.c | 2 +- >>> lib/packets.h | 2 ++ >>> 4 files changed, 18 insertions(+), 13 deletions(-) >>> >>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c >>> index 9abdf51076a8..db1c4c6d9bfc 100644 >>> --- a/lib/netdev-native-tnl.c >>> +++ b/lib/netdev-native-tnl.c >>> @@ -146,8 +146,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, >>> * >>> * Return pointer to the L4 header added to 'packet'. */ >>> void * >>> -netdev_tnl_push_ip_header(struct dp_packet *packet, >>> - const void *header, int size, int *ip_tot_size) >>> +netdev_tnl_push_ip_header(struct dp_packet *packet, const void *header, >>> + int size, int *ip_tot_size, ovs_be32 ipv6_label) >>> { >>> struct eth_header *eth; >>> struct ip_header *ip; >>> @@ -166,6 +166,7 @@ netdev_tnl_push_ip_header(struct dp_packet *packet, >>> ip6 = netdev_tnl_ipv6_hdr(eth); >>> *ip_tot_size -= IPV6_HEADER_LEN; >>> ip6->ip6_plen = htons(*ip_tot_size); >>> + packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); >> >> This function is on a hot path. It might make sense to update >> the flow label only if it is non-zero, to save some cycles. >> > > Hi Ilya, > > Thanks for your review. Yes, I agree. > I will simply insert an if block as follows: > > - packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); > + if (ipv6_label) { > + packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); > + } > If the implementation is to write an enum value in data->header at header build and replace it with the hash value at header push, I think it cannot be replaced by "if (ipv6_label) { ... }" block. The reason is that there are cases where an enum value is replaced by 0 in the cases srv6_flowlabel="copy" and srv6_flowlabel="zero". This would occur even if the order of the enum definitions is swapped. Please correct me if my understanding is incorrect.
On 5/22/23 10:17, Nobuhiro MIKI wrote: > On 2023/05/22 12:09, Nobuhiro MIKI wrote: >> On 2023/05/20 3:56, Ilya Maximets wrote: >>> On 5/16/23 07:33, Nobuhiro MIKI wrote: >>>> For tunnels such as SRv6, some popular vendor appliances support >>>> IPv6 flowlabel based load balancing. In preparation for OVS to >>>> support it, this patch modifies the encapsulation to allow IPv6 >>>> flowlabel to be configured. >>>> >>>> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> >>>> --- >>>> lib/netdev-native-tnl.c | 23 +++++++++++++---------- >>>> lib/netdev-native-tnl.h | 4 ++-- >>>> lib/packets.c | 2 +- >>>> lib/packets.h | 2 ++ >>>> 4 files changed, 18 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c >>>> index 9abdf51076a8..db1c4c6d9bfc 100644 >>>> --- a/lib/netdev-native-tnl.c >>>> +++ b/lib/netdev-native-tnl.c >>>> @@ -146,8 +146,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, >>>> * >>>> * Return pointer to the L4 header added to 'packet'. */ >>>> void * >>>> -netdev_tnl_push_ip_header(struct dp_packet *packet, >>>> - const void *header, int size, int *ip_tot_size) >>>> +netdev_tnl_push_ip_header(struct dp_packet *packet, const void *header, >>>> + int size, int *ip_tot_size, ovs_be32 ipv6_label) >>>> { >>>> struct eth_header *eth; >>>> struct ip_header *ip; >>>> @@ -166,6 +166,7 @@ netdev_tnl_push_ip_header(struct dp_packet *packet, >>>> ip6 = netdev_tnl_ipv6_hdr(eth); >>>> *ip_tot_size -= IPV6_HEADER_LEN; >>>> ip6->ip6_plen = htons(*ip_tot_size); >>>> + packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); >>> >>> This function is on a hot path. It might make sense to update >>> the flow label only if it is non-zero, to save some cycles. >>> >> >> Hi Ilya, >> >> Thanks for your review. Yes, I agree. >> I will simply insert an if block as follows: >> >> - packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); >> + if (ipv6_label) { >> + packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); >> + } >> > > If the implementation is to write an enum value in data->header at > header build and replace it with the hash value at header push, > I think it cannot be replaced by "if (ipv6_label) { ... }" block. > > The reason is that there are cases where an enum value is replaced > by 0 in the cases srv6_flowlabel="copy" and srv6_flowlabel="zero". > This would occur even if the order of the enum definitions is swapped. > Please correct me if my understanding is incorrect. Yeah, you're right. Keep it as-is then. Best regards, Ilya Maximets.
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index 9abdf51076a8..db1c4c6d9bfc 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -146,8 +146,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, * * Return pointer to the L4 header added to 'packet'. */ void * -netdev_tnl_push_ip_header(struct dp_packet *packet, - const void *header, int size, int *ip_tot_size) +netdev_tnl_push_ip_header(struct dp_packet *packet, const void *header, + int size, int *ip_tot_size, ovs_be32 ipv6_label) { struct eth_header *eth; struct ip_header *ip; @@ -166,6 +166,7 @@ netdev_tnl_push_ip_header(struct dp_packet *packet, ip6 = netdev_tnl_ipv6_hdr(eth); *ip_tot_size -= IPV6_HEADER_LEN; ip6->ip6_plen = htons(*ip_tot_size); + packet_set_ipv6_flow_label(&ip6->ip6_flow, ipv6_label); packet->l4_ofs = dp_packet_size(packet) - *ip_tot_size; return ip6 + 1; } else { @@ -245,7 +246,8 @@ netdev_tnl_push_udp_header(const struct netdev *netdev OVS_UNUSED, struct udp_header *udp; int ip_tot_size; - udp = netdev_tnl_push_ip_header(packet, data->header, data->header_len, &ip_tot_size); + udp = netdev_tnl_push_ip_header(packet, data->header, data->header_len, + &ip_tot_size, 0); /* set udp src port */ udp->udp_src = netdev_tnl_get_src_port(packet); @@ -456,7 +458,8 @@ netdev_gre_push_header(const struct netdev *netdev, struct gre_base_hdr *greh; int ip_tot_size; - greh = netdev_tnl_push_ip_header(packet, data->header, data->header_len, &ip_tot_size); + greh = netdev_tnl_push_ip_header(packet, data->header, data->header_len, + &ip_tot_size, 0); if (greh->flags & htons(GRE_CSUM)) { ovs_be16 *csum_opt = (ovs_be16 *) (greh + 1); @@ -611,8 +614,8 @@ netdev_erspan_push_header(const struct netdev *netdev, struct erspan_md2 *md2; int ip_tot_size; - greh = netdev_tnl_push_ip_header(packet, data->header, - data->header_len, &ip_tot_size); + greh = netdev_tnl_push_ip_header(packet, data->header, data->header_len, + &ip_tot_size, 0); /* update GRE seqno */ tnl_cfg = &dev->tnl_cfg; @@ -793,8 +796,8 @@ netdev_gtpu_push_header(const struct netdev *netdev, unsigned int payload_len; payload_len = dp_packet_size(packet); - udp = netdev_tnl_push_ip_header(packet, data->header, - data->header_len, &ip_tot_size); + udp = netdev_tnl_push_ip_header(packet, data->header, data->header_len, + &ip_tot_size, 0); udp->udp_src = netdev_tnl_get_src_port(packet); udp->udp_len = htons(ip_tot_size); netdev_tnl_calc_udp_csum(udp, packet, ip_tot_size); @@ -921,8 +924,8 @@ netdev_srv6_push_header(const struct netdev *netdev OVS_UNUSED, { int ip_tot_size; - netdev_tnl_push_ip_header(packet, data->header, - data->header_len, &ip_tot_size); + netdev_tnl_push_ip_header(packet, data->header, data->header_len, + &ip_tot_size, 0); } struct dp_packet * diff --git a/lib/netdev-native-tnl.h b/lib/netdev-native-tnl.h index 4dad8f978cc6..3311d796ed85 100644 --- a/lib/netdev-native-tnl.h +++ b/lib/netdev-native-tnl.h @@ -138,8 +138,8 @@ void * netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, unsigned int *hlen); void * -netdev_tnl_push_ip_header(struct dp_packet *packet, - const void *header, int size, int *ip_tot_size); +netdev_tnl_push_ip_header(struct dp_packet *packet, const void *header, + int size, int *ip_tot_size, ovs_be32 ipv6_label); void netdev_tnl_egress_port_range(struct unixctl_conn *conn, int argc, const char *argv[], void *aux OVS_UNUSED); diff --git a/lib/packets.c b/lib/packets.c index 06f516cb1af4..7e5a52fd40ed 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -1274,7 +1274,7 @@ packet_set_ipv6_addr(struct dp_packet *packet, uint8_t proto, pkt_metadata_init_conn(&packet->md); } -static void +void packet_set_ipv6_flow_label(ovs_16aligned_be32 *flow_label, ovs_be32 flow_key) { ovs_be32 old_label = get_16aligned_be32(flow_label); diff --git a/lib/packets.h b/lib/packets.h index 9465bec16c9c..ac4c28e471e6 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -1622,6 +1622,8 @@ void packet_set_ipv6_addr(struct dp_packet *packet, uint8_t proto, ovs_16aligned_be32 addr[4], const struct in6_addr *new_addr, bool recalculate_csum); +void packet_set_ipv6_flow_label(ovs_16aligned_be32 *flow_label, + ovs_be32 flow_key); void packet_set_tcp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst); void packet_set_udp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst); void packet_set_sctp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst);
For tunnels such as SRv6, some popular vendor appliances support IPv6 flowlabel based load balancing. In preparation for OVS to support it, this patch modifies the encapsulation to allow IPv6 flowlabel to be configured. Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> --- lib/netdev-native-tnl.c | 23 +++++++++++++---------- lib/netdev-native-tnl.h | 4 ++-- lib/packets.c | 2 +- lib/packets.h | 2 ++ 4 files changed, 18 insertions(+), 13 deletions(-)