[ovs-dev] [PATCH] OVN: add the possibility to specify tunnel dst port

[Ben Pfaff]

On Fri, Jun 28, 2019 at 03:08:31PM +0200, Lorenzo Bianconi wrote:
> >
> > On Tue, Jun 25, 2019 at 12:35:26PM +0200, Lorenzo Bianconi wrote:
> > > Introduce dst_port in options column of Encap table in order to add the
> > > capability to configure destination port used for tunnel encapsulation
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> >
> > Thank you.  I think that the documentation can be improved by using the
> > documentation system's ability to document individual keys.  Here is my
> > suggestion; I only changed the documentation.
> >
> > --8<--------------------------cut here-------------------------->8--
> >
> > From: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> > Date: Tue, 25 Jun 2019 12:35:26 +0200
> > Subject: [PATCH] OVN: add the possibility to specify tunnel dst port
> >
> > Introduce dst_port in options column of Encap table in order to add the
> > capability to configure destination port used for tunnel encapsulation
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> > ---
> >  ovn/controller/encaps.c |  4 ++++
> >  ovn/ovn-sb.xml          | 28 ++++++++++++++++++++--------
> >  2 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> > index 3b3921a736e2..d4a436df318c 100644
> > --- a/ovn/controller/encaps.c
> > +++ b/ovn/controller/encaps.c
> > @@ -156,6 +156,7 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
> >      struct smap options = SMAP_INITIALIZER(&options);
> >      smap_add(&options, "remote_ip", encap->ip);
> >      smap_add(&options, "key", "flow");
> > +    const char *dst_port = smap_get(&encap->options, "dst_port");
> >      const char *csum = smap_get(&encap->options, "csum");
> >      char *tunnel_entry_id = NULL;
> >
> > @@ -169,6 +170,9 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
> >      if (csum && (!strcmp(csum, "true") || !strcmp(csum, "false"))) {
> >          smap_add(&options, "csum", csum);
> >      }
> > +    if (dst_port) {
> > +        smap_add(&options, "dst_port", dst_port);
> > +    }
> >
> >      /* Add auth info if ipsec is enabled. */
> >      if (sbg->ipsec) {
> > diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> > index 1a2bc1da9ccc..5f7783e0de8e 100644
> > --- a/ovn/ovn-sb.xml
> > +++ b/ovn/ovn-sb.xml
> > @@ -362,14 +362,14 @@
> >      </column>
> >
> >      <column name="options">
> > -      <p>
> > -        Options for configuring the encapsulation. Currently, the only
> > -        option that has been defined is <code>csum</code>.
> > -      </p>
> > +      Options for configuring the encapsulation, which may be <ref column="type"/> specific.
> > +    </column>
> >
> > +    <column name="options" key="csum" type='{"type": "boolean"}'>
> >        <p>
> > -        <code>csum</code> indicates that encapsulation checksums can be
> > -        transmitted and received with reasonable performance. It is a hint
> > +        <code>csum</code> indicates whether this chassis can transmit and
> > +        receive packets that include checksums with reasonable performance.  It
> > +        hints
> >          to senders transmitting data to this chassis that they should use
> >          checksums to protect OVN metadata. <code>ovn-controller</code>
> >          populates this key with the value defined in
> > @@ -380,7 +380,7 @@
> >        </p>
> >
> >        <p>
> > -        In terms of performance, this actually significantly increases
> > +        In terms of performance, checksumming actually significantly increases
> >          throughput in most common cases when running on Linux based hosts
> >          without NICs supporting encapsulation hardware offload (around 60% for
> >          bulk traffic). The reason is that generally all NICs are capable of
> > @@ -399,7 +399,7 @@
> >          efficiently compute or validate full packet checksums. In addition
> >          certain versions of the Linux kernel are not able to fully take
> >          advantage of encapsulation NIC offloads in the presence of checksums.
> > -        (This is actually a pretty narrow corner case though - earlier
> > +        (This is actually a pretty narrow corner case though: earlier
> >          versions of Linux don't support encapsulation offloads at all and
> >          later versions support both offloads and checksums well.)
> >        </p>
> > @@ -408,6 +408,18 @@
> >          <code>csum</code> defaults to <code>false</code> for hardware VTEPs and
> >          <code>true</code> for all other cases.
> >        </p>
> > +
> > +      <p>
> > +        This option applies to <code>geneve</code> and <code>vxlan</code>
> > +        encapsulations.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="options" key="dst_port" type='{"type": "integer"}'>
> > +      <p>
> > +        If set, overrides the UDP (for <code>geneve</code> and
> > +        <code>vxlan</code>) or TCP (for <code>stt</code>) destination port.
> > +      </p>
> >      </column>
> >
> >      <column name="ip">
> > --
> > 2.20.1
> >
> 
> Hi Ben,
> 
> I agree it is definitely better :) Do I need to resend?

No.  Since you approve, I added Numan's ack and applied this to master.
Thanks again!