[ovs-dev] [PATCH v2 1/5] Enable kernel probes and map stream probes onto them

Anton Ivanov anton.ivanov at cambridgegreys.com
Thu Jun 4 15:11:01 UTC 2020


On 04/06/2020 15:43, Mark Michelson wrote:
> Hi Anton, a couple of comments in-line below.
>
> On 6/2/20 3:21 AM, anton.ivanov at cambridgegreys.com wrote:
>> From: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>>
>> 1. Fix probe logic. The stream_or_pstream_needs_probes
>> function returning a mix of integer and boolean. As a
>> result probes were NOT turned off in a number of cases on
>> unix domain sockets and other transports where there
>> should be no probing. It now returns -1 (do not know),
>> 0 (no probe needed), 1 (definitely needs probes).
>
> In the patch, a return of -1 and a return of 0 are always treated the same. I think a simpler fix here would be to change the -1 return in stream_or_pstream_needs_probes() to a 0 return instead. This way, the code can continue treating a non-zero return as needing probes instead of using a magic number comparison.

Ack.

I did not want to lose the "I do not know" case in case someone needs it in the future. For the time being nobody does so we can indeed return 0 in needs_probes.

I will update the patch in the next revision.

>
>>
>> 2. Allow delegating probing to keepalive facilities in the
>> stream layer if avaialable.
>>
>> 3. Provide TCP KEEPALIVE probing at stream layer on supported
>> platforms for stream-ssl and stream-tcp.
>>
>> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>> ---
>>   lib/jsonrpc.c          | 36 ++++++++++++++++++++++++++++++++--
>>   lib/rconn.c            |  2 +-
>>   lib/socket-util.c      | 44 ++++++++++++++++++++++++++++++++++++++++++
>>   lib/socket-util.h      |  8 ++++++++
>>   lib/stream-fd.c        |  9 +++++++++
>>   lib/stream-provider.h  |  6 ++++++
>>   lib/stream-ssl.c       |  8 ++++++++
>>   lib/stream-tcp.c       |  1 +
>>   lib/stream-unix.c      |  1 +
>>   lib/stream-windows.c   |  1 +
>>   lib/stream.c           | 17 ++++++++++++++--
>>   lib/stream.h           |  1 +
>>   ovsdb/jsonrpc-server.c |  2 +-
>>   13 files changed, 130 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
>> index ed748dbde..15e8d3527 100644
>> --- a/lib/jsonrpc.c
>> +++ b/lib/jsonrpc.c
>> @@ -787,6 +787,7 @@ struct jsonrpc_session {
>>       int last_error;
>>       unsigned int seqno;
>>       uint8_t dscp;
>> +    int probe_interval;
>>   };
>>     static void
>> @@ -839,6 +840,7 @@ jsonrpc_session_open_multiple(const struct svec *remotes, bool retry)
>>       s->seqno = 0;
>>       s->dscp = 0;
>>       s->last_error = 0;
>> +    s->probe_interval = reconnect_get_probe_interval(s->reconnect);
>>         const char *name = reconnect_get_name(s->reconnect);
>>       if (!pstream_verify_name(name)) {
>> @@ -848,8 +850,9 @@ jsonrpc_session_open_multiple(const struct svec *remotes, bool retry)
>>           reconnect_set_backoff(s->reconnect, INT_MAX, INT_MAX);
>>       }
>>   -    if (!stream_or_pstream_needs_probes(name)) {
>> +    if (stream_or_pstream_needs_probes(name) < 1) {
>>           reconnect_set_probe_interval(s->reconnect, 0);
>> +        s->probe_interval = 0;
>>       }
>>         return s;
>> @@ -879,6 +882,12 @@ jsonrpc_session_open_unreliably(struct jsonrpc *jsonrpc, uint8_t dscp)
>>       s->stream = NULL;
>>       s->pstream = NULL;
>>       s->seqno = 1;
>> +    s->probe_interval = reconnect_get_probe_interval(s->reconnect);
>> +
>> +    if (stream_or_pstream_needs_probes(reconnect_get_name(s->reconnect)) < 1) {
>> +        reconnect_set_probe_interval(s->reconnect, 0);
>> +        s->probe_interval = 0;
>> +    }
>>         return s;
>>   }
>> @@ -934,6 +943,12 @@ jsonrpc_session_connect(struct jsonrpc_session *s)
>>           error = jsonrpc_stream_open(name, &s->stream, s->dscp);
>>           if (!error) {
>>               reconnect_connecting(s->reconnect, time_msec());
>> +            if (stream_set_probe_interval(s->stream, s->probe_interval)) {
>> +                /* we have delegated probing to the stream layer */
>> +                reconnect_set_probe_interval(s->reconnect, 0);
>> +            } else {
>> +                reconnect_set_probe_interval(s->reconnect, s->probe_interval);
>> +            }
>>           } else {
>>               s->last_error = error;
>>           }
>> @@ -967,6 +982,12 @@ jsonrpc_session_run(struct jsonrpc_session *s)
>>                   jsonrpc_session_disconnect(s);
>>               }
>>               reconnect_connected(s->reconnect, time_msec());
>> +            if (stream_set_probe_interval(stream, s->probe_interval)) {
>> +                /* we have delegated probing to the stream layer */
>> +                reconnect_set_probe_interval(s->reconnect, 0);
>> +            } else {
>> +                reconnect_set_probe_interval(s->reconnect, s->probe_interval);
>> +            }
>>               s->rpc = jsonrpc_open(stream);
>>               s->seqno++;
>>           } else if (error != EAGAIN) {
>> @@ -1008,6 +1029,12 @@ jsonrpc_session_run(struct jsonrpc_session *s)
>>           if (!error) {
>>               reconnect_connected(s->reconnect, time_msec());
>>               s->rpc = jsonrpc_open(s->stream);
>> +            if (stream_set_probe_interval(s->stream, s->probe_interval)) {
>> +                /* we have delegated probing to the stream layer */
>> +                reconnect_set_probe_interval(s->reconnect, 0);
>> +            } else {
>> +                reconnect_set_probe_interval(s->reconnect, s->probe_interval);
>> +            }
>>               s->stream = NULL;
>>               s->seqno++;
>>           } else if (error != EAGAIN) {
>> @@ -1231,7 +1258,12 @@ void
>>   jsonrpc_session_set_probe_interval(struct jsonrpc_session *s,
>>                                      int probe_interval)
>>   {
>> -    reconnect_set_probe_interval(s->reconnect, probe_interval);
>> +    s->probe_interval = probe_interval;
>> +    if (s->stream && s->probe_interval) {
>> +        if (!stream_set_probe_interval(s->stream, probe_interval)) {
>> +           reconnect_set_probe_interval(s->reconnect, probe_interval);
>
> I might be missing something, but the logic seems a bit off here.
>
> 1) If probe_interval is 0, then this doesn't alter probe intervals at all. The comment above the function says that a 0 value should disable probe intervals.
> 2) If s->stream is NULL, then this won't alter s->reconnect's probe_interval at all. This could be an issue if the intent was to lengthen the probe interval or disable it temporarily during downtime.
> 3) In the case where stream_set_probe_interval succeeds, shouldn't we call reconnect_set_probe_interval(s->reconnect, 0)?
>
>> +        }
>> +    }
>>   }
>>     /* Sets the DSCP value used for 's''s connection to 'dscp'. If this is
>> diff --git a/lib/rconn.c b/lib/rconn.c
>> index a96b2eb8b..16e7a44d4 100644
>> --- a/lib/rconn.c
>> +++ b/lib/rconn.c
>> @@ -346,7 +346,7 @@ rconn_connect(struct rconn *rc, const char *target, const char *name)
>>       rconn_disconnect__(rc);
>>       rconn_set_target__(rc, target, name);
>>       rc->reliable = true;
>> -    if (!stream_or_pstream_needs_probes(target)) {
>> +    if (stream_or_pstream_needs_probes(target) < 1) {
>>           rc->probe_interval = 0;
>>       }
>>       reconnect(rc);
>> diff --git a/lib/socket-util.c b/lib/socket-util.c
>> index 4f1ffecf5..d6d1967f6 100644
>> --- a/lib/socket-util.c
>> +++ b/lib/socket-util.c
>> @@ -114,6 +114,50 @@ setsockopt_tcp_nodelay(int fd)
>>       }
>>   }
>>   +#ifdef HAS_KERNEL_KEEPALIVES
>> +bool tcp_set_probe_interval(int fd, int probe_interval) {
>
> Is there any way to attempt to make this function atomic? For example, we could manage to turn on keepalives, successfully set TCP_KEEPCNT, but then fail to set TCP_KEEPIDLE. This would result in a false return, which callers currently interpret to mean a complete failure to set keepalives. However, in actuality they've been enabled but just not with the configured probe_interval. This could lead to some odd debugging scenarios.
>
> Also, there should be a special case in this function where if probe_interval is 0, it disables keepalives. I have no idea what happens if you turn on keepalives but set TCP_KEEPIDLE and TCP_KEEPINTVL to 0.
>
>> +    int on = 1;
>> +    int retval;
>> +    int value;
>> +
>> +    on = 1;
>> +    retval = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &on, sizeof on);
>> +    if (retval) {
>> +        retval = sock_errno();
>> +        VLOG_DBG("setsockopt(SO_KEEPALIVE): %s", sock_strerror(retval));
>> +        return false;
>> +    }
>> +    value = 2;
>> +    retval = setsockopt(fd,
>> +            IPPROTO_TCP, TCP_KEEPCNT, &value, sizeof value);
>> +    if (retval) {
>> +        retval = sock_errno();
>> +        VLOG_DBG("setsockopt(TCP_KEEPCNT): %s", sock_strerror(retval));
>> +        return false;
>> +    }
>> +    value = probe_interval;
>> +    retval = setsockopt(fd,
>> +            IPPROTO_TCP, TCP_KEEPIDLE, &value, sizeof value);
>> +    if (retval) {
>> +        retval = sock_errno();
>> +        VLOG_DBG("setsockopt(TCP_KEEPIDLE): %s", sock_strerror(retval));
>> +        return false;
>> +    }
>> +    value = probe_interval;
>> +    retval = setsockopt(fd,
>> +            IPPROTO_TCP, TCP_KEEPINTVL, &value, sizeof value);
>> +    if (retval) {
>> +        retval = sock_errno();
>> +        VLOG_DBG("setsockopt(SO_KEEPALIVE): %s", sock_strerror(retval));
>> +        return false;
>> +    }
>> +    return true;
>> +#else
>> +bool tcp_set_probe_interval(int fd OVS_UNUSED, int probe_interval OVS_UNUSED) {
>> +    return false;
>> +#endif
>> +}
>> +
>>   /* Sets the DSCP value of socket 'fd' to 'dscp', which must be 63 or less.
>>    * 'family' must indicate the socket's address family (AF_INET or AF_INET6, to
>>    * do anything useful). */
>> diff --git a/lib/socket-util.h b/lib/socket-util.h
>> index 9ccb7d4cc..2439e2f78 100644
>> --- a/lib/socket-util.h
>> +++ b/lib/socket-util.h
>> @@ -27,6 +27,7 @@
>>   #include "openvswitch/types.h"
>>   #include <netinet/in_systm.h>
>>   #include <netinet/ip.h>
>> +#include <netinet/tcp.h>
>>     struct ds;
>>   @@ -181,4 +182,11 @@ static inline int sock_errno(void)
>>   #endif
>>   }
>>   +#if defined (SO_KEEPALIVE) && defined (TCP_KEEPCNT) && \
>> +    defined (TCP_KEEPIDLE) && defined (TCP_KEEPINTVL)
>> +#define HAS_KERNEL_KEEPALIVES 1
>> +#endif
>> +
>> +bool tcp_set_probe_interval(int fd, int probe_interval);
>> +
>>   #endif /* socket-util.h */
>> diff --git a/lib/stream-fd.c b/lib/stream-fd.c
>> index 46ee7ae27..30622929b 100644
>> --- a/lib/stream-fd.c
>> +++ b/lib/stream-fd.c
>> @@ -162,6 +162,14 @@ fd_wait(struct stream *stream, enum stream_wait_type wait)
>>       }
>>   }
>>   +static bool fd_set_probe_interval(struct stream *stream, int probe_interval) {
>> +    struct stream_fd *sf = stream_fd_cast(stream);
>> +
>> +    return tcp_set_probe_interval(sf->fd, probe_interval);
>> +}
>> +
>> +
>> +
>>   static const struct stream_class stream_fd_class = {
>>       "fd",                       /* name */
>>       false,                      /* needs_probes */
>> @@ -173,6 +181,7 @@ static const struct stream_class stream_fd_class = {
>>       NULL,                       /* run */
>>       NULL,                       /* run_wait */
>>       fd_wait,                    /* wait */
>> +    fd_set_probe_interval,      /* set_probe_interval */
>>   };
>>   

>>   /* Passive file descriptor stream. */
>> diff --git a/lib/stream-provider.h b/lib/stream-provider.h
>> index 75f4f059b..6c28cb50b 100644
>> --- a/lib/stream-provider.h
>> +++ b/lib/stream-provider.h
>> @@ -124,6 +124,12 @@ struct stream_class {
>>       /* Arranges for the poll loop to wake up when 'stream' is ready to take an
>>        * action of the given 'type'. */
>>       void (*wait)(struct stream *stream, enum stream_wait_type type);
>> +    /* Sets low level keepalives if supported
>> +     *
>> +     *     If successful returns true
>> +     *
>> +     */
>> +    bool (*set_probe_interval)(struct stream *stream, int probe_interval);
>>   };
>>   

>>   /* Passive listener for incoming stream connections.
>> diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
>> index 078fcbc3a..575c55f5b 100644
>> --- a/lib/stream-ssl.c
>> +++ b/lib/stream-ssl.c
>> @@ -789,6 +789,13 @@ ssl_run(struct stream *stream)
>>       }
>>   }
>>   +static bool ssl_set_probe_interval(struct stream *stream, int probe_interval) {
>> +    struct ssl_stream *sslv = ssl_stream_cast(stream);
>> +
>> +    return tcp_set_probe_interval(sslv->fd, probe_interval);
>> +}
>> +
>> +
>>   static void
>>   ssl_run_wait(struct stream *stream)
>>   {
>> @@ -861,6 +868,7 @@ const struct stream_class ssl_stream_class = {
>>       ssl_run,                    /* run */
>>       ssl_run_wait,               /* run_wait */
>>       ssl_wait,                   /* wait */
>> +    ssl_set_probe_interval,     /* set_probe_interval */
>>   };
>>   

>>   /* Passive SSL. */
>> diff --git a/lib/stream-tcp.c b/lib/stream-tcp.c
>> index e8dc2bfaa..67c912105 100644
>> --- a/lib/stream-tcp.c
>> +++ b/lib/stream-tcp.c
>> @@ -73,6 +73,7 @@ const struct stream_class tcp_stream_class = {
>>       NULL,                       /* run */
>>       NULL,                       /* run_wait */
>>       NULL,                       /* wait */
>> +    NULL,
>>   };
>>   

>>   /* Passive TCP. */
>> diff --git a/lib/stream-unix.c b/lib/stream-unix.c
>> index d265efb83..2322df955 100644
>> --- a/lib/stream-unix.c
>> +++ b/lib/stream-unix.c
>> @@ -73,6 +73,7 @@ const struct stream_class unix_stream_class = {
>>       NULL,                       /* run */
>>       NULL,                       /* run_wait */
>>       NULL,                       /* wait */
>> +    NULL,
>>   };
>>   

>>   /* Passive UNIX socket. */
>> diff --git a/lib/stream-windows.c b/lib/stream-windows.c
>> index 5c4c55e5d..836112f75 100644
>> --- a/lib/stream-windows.c
>> +++ b/lib/stream-windows.c
>> @@ -374,6 +374,7 @@ const struct stream_class windows_stream_class = {
>>       NULL,                       /* run */
>>       NULL,                       /* run_wait */
>>       windows_wait,               /* wait */
>> +    NULL,
>>   };
>>     struct pwindows_pstream
>> diff --git a/lib/stream.c b/lib/stream.c
>> index e246b3773..f0b16fb4f 100644
>> --- a/lib/stream.c
>> +++ b/lib/stream.c
>> @@ -430,6 +430,19 @@ stream_wait(struct stream *stream, enum stream_wait_type wait)
>>       (stream->class->wait)(stream, wait);
>>   }
>>   +
>> +bool stream_set_probe_interval(struct stream *stream, int probe_interval) {
>> +    if (! stream->class->needs_probes) {
>> +        return true;
>> +    }
>> +    if (probe_interval && stream->class->set_probe_interval) {
>> +        return (stream->class->set_probe_interval)(
>> +                stream, probe_interval / 1000);
>> +    }
>> +    return false;
>> +}
>> +
>> +
>>   void
>>   stream_connect_wait(struct stream *stream)
>>   {
>> @@ -509,9 +522,9 @@ stream_or_pstream_needs_probes(const char *name)
>>       const struct stream_class *class;
>>         if (!stream_lookup_class(name, &class)) {
>> -        return class->needs_probes;
>> +        return class->needs_probes ? 1 : 0;
>>       } else if (!pstream_lookup_class(name, &pclass)) {
>> -        return pclass->needs_probes;
>> +        return pclass->needs_probes ? 1 : 0;
>>       } else {
>>           return -1;
>>       }
>> diff --git a/lib/stream.h b/lib/stream.h
>> index 77bffa498..e343f75a5 100644
>> --- a/lib/stream.h
>> +++ b/lib/stream.h
>> @@ -40,6 +40,7 @@ const char *stream_get_name(const struct stream *);
>>   int stream_connect(struct stream *);
>>   int stream_recv(struct stream *, void *buffer, size_t n);
>>   int stream_send(struct stream *, const void *buffer, size_t n);
>> +bool stream_set_probe_interval(struct stream *, int probe_interval);
>>     void stream_run(struct stream *);
>>   void stream_run_wait(struct stream *);
>> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
>> index 4e2dfc3d7..fe8ffc317 100644
>> --- a/ovsdb/jsonrpc-server.c
>> +++ b/ovsdb/jsonrpc-server.c
>> @@ -212,7 +212,7 @@ ovsdb_jsonrpc_default_options(const char *target)
>>   {
>>       struct ovsdb_jsonrpc_options *options = xzalloc(sizeof *options);
>>       options->max_backoff = RECONNECT_DEFAULT_MAX_BACKOFF;
>> -    options->probe_interval = (stream_or_pstream_needs_probes(target)
>> +    options->probe_interval = (stream_or_pstream_needs_probes(target) < 1
>>                                  ? RECONNECT_DEFAULT_PROBE_INTERVAL
>>                                  : 0);
>>       return options;
>>
>
>
-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/



More information about the dev mailing list