[vlc-devel] [PATCH 3/3] access/udp: reduce mtu amount on initial packets

[Nick Briggs]

> Date: Mon, 15 Oct 2018 21:30:53 +0300
> From: Ilkka Ollakka <ileoo at videolan.org>
> To: vlc-devel at videolan.org
> Subject: [vlc-devel] [PATCH 3/3] access/udp: reduce mtu amount on
> 	initial packets
> Message-ID: <20181015183053.27240-3-ileoo at videolan.org>
> 
> In this approach, mtu is setted to high on start, and if received data
> is less than 1500 and mtu is higher than that, we adjust mtu on future
> packets to be smaller. In majority of cases in UDP streaming, packet
> size is usually quite constant, so it should work, even if it's not
> generic solution to udp packet receiving.
> 
> Also in case of truncing packets on other than linux systems, use same
> assumption and peek the next data amount to adjust mtu for future
> packets. We don't call PEEK on all the cases, as it would be unnecessary
> system call on majority of the calls.
> 
> ref #20952 and other similar tickets.
> ---
> modules/access/udp.c | 40 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/modules/access/udp.c b/modules/access/udp.c
> index b5ba292e48..7648b2ebc6 100644
> --- a/modules/access/udp.c
> +++ b/modules/access/udp.c
> @@ -168,7 +168,7 @@ static int Open( vlc_object_t *p_this )
>         return VLC_EGENERIC;
>     }
> 
> -    sys->mtu = 7 * 188;
> +    sys->mtu = 65536;
> 
>     sys->timeout = var_InheritInteger( p_access, "udp-timeout");
>     if( sys->timeout > 0)
> @@ -262,6 +262,7 @@ static block_t *BlockUDP(stream_t *access, bool *restrict eof)
>             goto skip;
>      }
> 
> +
>     ssize_t len = recvmsg(sys->fd, &msg, trunc_flag);
> 
>     if (len < 0)
> @@ -271,14 +272,47 @@ skip:
>         return NULL;
>     }
> 
> -    if (msg.msg_flags & trunc_flag )
> +    /* Check if received amount is other side of ethernet mtu amount of 1500,
> +       and reduce mtu if it's so. Failsafe to keep mtu still atleast 1000 bytes.
> +       With this approach, we can start with way higher than needed mtu for 1 packet
> +       and with first packets, it should adjust to reasonable size.
> +       Assuming that most cases in real world the udp packet size don't fluctuate
> +       huge amounts on size in streaming. And on those cases the MSG_TRUNC should
> +       then readjust.
> +     */
> +
> +    if (unlikely((sys->mtu > 1500) && (len > 1000) && (len < 1500)))
>     {
> +        msg_Dbg(access, "adjusting MTU from %zd to %zu", sys->mtu, len);
> +        sys->mtu = len;
> +    }
> +
> +#ifdef MSG_TRUNC
> +    if (msg.msg_flags & MSG_TRUNC)
> +    {
> +        pkt->i_flags |= BLOCK_FLAG_CORRUPTED;
> +        /* On linux systems MSG_TRUNC tells the available data,
> +           on other cases, I think we need to peek to get how much
> +           there would be availabled in case of trunced message.
> +           In MSG_PEEK case, assumption is also, that in real world
> +           the size don't fluctuate huge amounts in streaming.
> +         */
> +#ifdef __linux__
>         msg_Err(access, "%zd bytes packet truncated (MTU was %zu)",
>                 len, sys->mtu);
> -        pkt->i_flags |= BLOCK_FLAG_CORRUPTED;
>         sys->mtu = len;
> +#else
> +        sys->mtu = recv(sys->fd, pkt->p_buffer, sys->mtu, MSG_PEEK|MSG_TRUNC);

Perhaps I'm confused, but on a non-Linux system (MacOS, [Free|Open]BSD) MSG_TRUNC is not an input flag for a recv() call.
On those systems MSG_TRUNC is only ever an output flag returned in the msghdr.msg_flags.

> +        /* In this case, len is old mtu and max received amount, mtu is the
> +         * peeked value, that's why they are otherway around in this compared
> +         * to __linux__ case error
> +         */
> +        msg_Err(access, "%zd bytes packet truncated (MTU was %zu)",
> +                sys->mtu, len);
> +#endif
>     }
>     else
> +#endif
>         pkt->i_buffer = len;
> 
>     return pkt;
> -- 
> 2.19.0
> 
> 
> 
> ------------------------------
> 
> Message: 14
> Date: Mon, 15 Oct 2018 21:30:52 +0300
> From: Ilkka Ollakka <ileoo at videolan.org>
> To: vlc-devel at videolan.org
> Subject: [vlc-devel] [PATCH 2/3] access/udp: Define MSG_TRUNC on recv
> 	call
> Message-ID: <20181015183053.27240-2-ileoo at videolan.org>
> 
> recv() needs MSG_TRUNC flag so it will tell actual data-size if truncate
> would happen additional to setting flag for it.
For Linux, but not for MacOS/FreeBSD/OpenBSD

> 
> Tested adjustment of MTU by sending data to localhost via netcat:
> 
> cat ~/tmp/myfile.mp3|netcat -u 127.0.0.1 1234
> 
> and vlc -Idummy -vv udp://@127.0.0.1:1234 --sout="#stat"
> 
> without this patch the udp input constantly complains about trunced
> input and doesn't adjust the receiving amount.
> 
> udp fixup
> ---
> modules/access/udp.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/modules/access/udp.c b/modules/access/udp.c
> index d84cb7b99a..b5ba292e48 100644
> --- a/modules/access/udp.c
> +++ b/modules/access/udp.c
> @@ -231,6 +231,12 @@ static block_t *BlockUDP(stream_t *access, bool *restrict eof)
>         return NULL;
>     }
> 
> +#ifdef __linux__
> +    const int trunc_flag = MSG_TRUNC;
> +#else
> +    const int trunc_flag = 0;
> +#endif
> +

MacOS has the MSG_TRUNC flag, and if you do this you won't be able to check the msg.msg_flags returned value later -- is that what you intended?

>     struct iovec iov = {
>         .iov_base = pkt->p_buffer,
>         .iov_len = sys->mtu,
> @@ -238,9 +244,7 @@ static block_t *BlockUDP(stream_t *access, bool *restrict eof)
>     struct msghdr msg = {
>         .msg_iov = &iov,
>         .msg_iovlen = 1,
> -#ifdef __linux__
> -        .msg_flags = MSG_TRUNC,
> -#endif
> +        .msg_flags = trunc_flag,

Isn't this an output only field? At least that's what the documentation implies even on Linux.

>     };
> 
>     struct pollfd ufd[1];
> @@ -258,7 +262,8 @@ static block_t *BlockUDP(stream_t *access, bool *restrict eof)
>             goto skip;
>      }
> 
> -    ssize_t len = recvmsg(sys->fd, &msg, 0);
> +    ssize_t len = recvmsg(sys->fd, &msg, trunc_flag);
> +
>     if (len < 0)
>     {
> skip:
> @@ -266,8 +271,7 @@ skip:
>         return NULL;
>     }
> 
> -#ifdef MSG_TRUNC
> -    if (msg.msg_flags & MSG_TRUNC)
> +    if (msg.msg_flags & trunc_flag )
>     {
>         msg_Err(access, "%zd bytes packet truncated (MTU was %zu)",
>                 len, sys->mtu);
> @@ -275,7 +279,6 @@ skip:
>         sys->mtu = len;
>     }
>     else
> -#endif
>         pkt->i_buffer = len;
> 
>     return pkt;
> -- 
> 2.19.0
>