Request: Revert some changes on commit 2fb66a7

[lars18th]

Hi @catalinii ,

I have some comments about changes in this commit: https://github.com/catalinii/minisatip/commit/2fb66a703e0d00d7eb99222abc704bb05a18bc0a

One of the changes is related to part of my fix in PR #997. I've spend years of use to discover that the satipc module will not work sufficient reliable because the receive buffer has not updated with a correct size. The default network buffer size is incompatible with the work done by the satipc. Futhermore, the dvb module continues having the option to set the buffer size. And this has sense because the user requires to tweak it based on the physical hardware used. However, for the satipc (in my environment the most relevant and important part of the tool) you've now removed the option to adjust correctly the size of the DVR reading buffer. The commit log indicates that the user can adjust it using system variables (/proc/sys/net/core/rmem_default and /proc/sys/net/core/wmem_default). But this is a false statement. Almost I see these cases in which is false:

• When minisatip is running inside a server not administred by the user.
• When minisatip is running in a Docker container.
• When minisatip is running in a server shared with other services that require different values for these system variables.

And perhaps I forgot to mention some other cases.

So, please consider one of these two options: 1) Revert the part of this commit that removes the satipc adjust to the DVR buffer. In fact the lines: 571-572 and 589-590. 2) Or add the option to adjust independently the DVB read buffer and the SATIPC read buffer.

Sorry if this sounds quite rude. It's not my objective to say something incorrect. I only want to comment that this generates a lot of side effects that perhaps you don't have considered. And perhaps the best is to implement the option 2. What you think?

Regards.

[catalinii]

@lars18th opposing opinions as long as they are respectful are quite welcome.

The reason I removed those lines are because I noticed issues in my setup. My DVB hardware using minisatip server is 190ms away from the minisatip satipc. I have optic fiber on both ends with no bandwidth issues in between.

If you look at the kernel code: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/core/sock.c?h=v6.2-rc4#n1638 SO_RVCBUF sets sk->sk_rcvbuf, for which the default value is rmem_default: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/core/sock.c?h=v6.2-rc4#n3390. For tcp, this is set here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv4/tcp.c#n458

What I am trying to say is that those values can be based on the sysctl values. Try it on your setup and let me know if it works.

In all 3 cases increasing the buffers leads just to increased memory usage. Here we are talking about MBs of memory and all the systems today have that. For example my gbutraueh handles this with the following settings:

cat /proc/sys/net/ipv4/tcp_rmem

4096 87380 2472960 On the other systems I use larger rmem settings:

cat /proc/sys/net/ipv4/tcp_rmem

10240 87380 12582912

If we think about minisatip, it does set the socket buffer value to whatever the DVR_buffer is supposed to be set at.

For send buffer, I think increasing the default buffer size is detrimental, because minisatip has the ability of pushing rtsp replies ahead of buffered TS data. If you use a large buffer on the kernel size, then the kernel will flush the kernel buffer first before flushing the RTSP replies which could lead to RTSP session time out.

With a system that is not overloaded the application will anyway have the chance of reading the entire data in the adapter buffer before the socket buffer is full on the kernel side.

I really think that this should not be set by default and if you really think is needed we can add it as a long option for the satip client.

[lars18th]

Hi @catalinii ,

Thank you for exposing your opinion. I agree in part, but I need to add this:

• In my current server I can't change these sysctl values. The reason is because is a shared server and I don't have the option to tweak them.
• With the current default values I suffer for a lot of troubles when the bitrate changes abrutly using the satipc module.
• Only with an specific control of these buffers I'll be confortable.

Therefore, I suggest this:

• Restore the code of the functions set_socket_send_buffer() and set_socket_receive_buffer().
• Change the parameter -b --buffer X:Y:Z to add the value Z that will be the size of the SATIPC reading socket buffer. And if the value is 0 (the default value if not provided) then the default socket buffer size will be used automatically (that's not call to set_socket_send_buffer()).
• Change the parameter -B X:Y to add the value Y as the opts.output_buffer. And instead of the current default in the code of opts.output_buffer = 2 * 1024 * 1024; use the same idea of a default value of 0 indicating to not call to the set socket function.

You agree with this balanced option?

[Jalle19]

I'm not sure if "I don't have root access" should be a supported scenario. I suspect the vast majority of users are able to set sysctl variables.

[lars18th]

Hi @Jalle19 ,

I'm not sure if "I don't have root access" should be a supported scenario. I suspect the vast majority of users are able to set sysctl variables.

I feel you're thinking only on running minisatip on STB devices. But this is not only the unique platform to run it. When using the SATIPC module (the root cause for requesting the rollback of this commit) is more common to run minisatip inside a server. And this server could be shared with other services. Think for example when you run minisatip inside a Docker container. In this case you don't have the capabilities to change any sysctl value.

Anyway, what I really think that doesn't have sense is to remove something that is not causing any trouble at all to any group of users. With the addition that removing it will cause troubles to some users (myself included in this group). So if something requires to be changed, then I can accept it... if some balanced solution will be provided. For example, the addition of the -b X:Y:Z and -B X:Y values seems to be simple and it adds support for all users and environments. So why not use this solution?

[catalinii]

@lars18th we can add a new long config option if you want...let's not tie it to the DVR buffer from the kernel

[lars18th]

we can add a new long config option if you want...let's not tie it to the DVR buffer from the kernel

OK. I'll do it by myself. However, they're two buffer sizes: satipc receive buffer and send client buffer. For the sencond, you agree to use the parameter -B X:Y? And for the first: you prefer --satipc-buffer X instead of -b X:Y:Z (that is my prefered option)?

[Jalle19]

I feel you're thinking only on running minisatip on STB devices. But this is not only the unique platform to run it. When using the SATIPC module (the root cause for requesting the rollback of this commit) is more common to run minisatip inside a server. And this server could be shared with other services. Think for example when you run minisatip inside a Docker container. In this case you don't have the capabilities to change any sysctl value.

People have root to their own servers too, not just their STBs.

[lars18th]

Hi @Jalle19 ,

People have root to their own servers too, not just their STBs.

Please, with all due respect, stop to comment about this. I'm one of the users that runs minisatip in a server without root privileges or without option to change these sysctl kernel values. And not only in one way: one is running inside a Docker server; another inside a NAS; and another one in a shared x86 server. In all three environments the option of tweak the buffer using sysctl values is impossible.

Fortunately @catalinii accepts to incorporate support for configure these values in parameters. So for me it's a good solution. And no user will be harmed.

[catalinii]

--satip-receive-buffer should work.

Meanwhile can you provide the output of "ss -i" on the satip server when the congestion happens ? This is with the current master

[catalinii]

BTW, based on. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv4/tcp_input.c#n582 If I understand the code right the receive buffer will grow automatically up until the tcp_wmem max (last value). So I am not convinced that it does something for your use case. If you are running as root then indeed you can go over tcp_wmem max, but then changing the sysctl is also an option. Please provide the output of "ss -i" and check only the output related to the satip server socket (do not run it on the satip client but on the satip server host).

[lars18th]

Hi @catalinii ,

All work done in PR #1038. Please check it and merge. Because the default behaviour will be to set de options as 0, then the setsockopt() will be only called if the user wants to tweak these buffers manually.

Please merge it soon because my environment requires it for the satipc module (and I prefer to use the mainstream version). Regards.

[lars18th]

Hi,

As the PR #1038 is merged, then I close this issue.