diff mbox series

net/core/dev.c: enable timestamp static key if CPU isolation is configured

Message ID ZdSAWAwUxc5R46NH@tpad
State New
Headers
Series net/core/dev.c: enable timestamp static key if CPU isolation is configured |

Commit Message

Marcelo Tosatti Feb. 20, 2024, 10:35 a.m. UTC 
  For systems that use CPU isolation (via nohz_full), creating or destroying
a socket with  timestamping (SOF_TIMESTAMPING_OPT_TX_SWHW) might cause a
static key to be enabled/disabled. This in turn causes undesired 
IPIs to isolated CPUs.

So enable the static key unconditionally, if CPU isolation is enabled,
thus avoiding the IPIs.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Comments

Paolo Abeni Feb. 22, 2024, 9:34 a.m. UTC | #1 
On Tue, 2024-02-20 at 07:35 -0300, Marcelo Tosatti wrote:
> For systems that use CPU isolation (via nohz_full), creating or destroying
> a socket with  timestamping (SOF_TIMESTAMPING_OPT_TX_SWHW) might cause a
> static key to be enabled/disabled. This in turn causes undesired 
> IPIs to isolated CPUs.
> 
> So enable the static key unconditionally, if CPU isolation is enabled,
> thus avoiding the IPIs.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

This looks like net-next material, but it does not apply cleanly to the
net-next tree. Could you please rebase and repost including the 'net-
next' tag into the patch prefix?

Thanks!

Paolo
Willem de Bruijn Feb. 22, 2024, 3:11 p.m. UTC | #2 
Marcelo Tosatti wrote:
> For systems that use CPU isolation (via nohz_full), creating or destroying
> a socket with  timestamping (SOF_TIMESTAMPING_OPT_TX_SWHW) might cause a
> static key to be enabled/disabled. This in turn causes undesired 
> IPIs to isolated CPUs.

This refers to SOF_TIMESTAMPING_RX_SOFTWARE, not SOF_TIMESTAMPING_OPT_TX_SWHW.
See also sock_set_timestamping.
> 
> So enable the static key unconditionally, if CPU isolation is enabled,
> thus avoiding the IPIs.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0d548431f3fa..cc9a77b4aa4e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -153,6 +153,7 @@
>  #include <linux/prandom.h>
>  #include <linux/once_lite.h>
>  #include <net/netdev_rx_queue.h>
> +#include <linux/sched/isolation.h>
>  
>  #include "dev.h"
>  #include "net-sysfs.h"
> @@ -11601,3 +11602,14 @@ static int __init net_dev_init(void)
>  }
>  
>  subsys_initcall(net_dev_init);
> +
> +static int __init net_dev_late_init(void)
> +{
> +	/* avoid static key IPIs to isolated CPUs */
> +	if (housekeeping_enabled(HK_TYPE_MISC))
> +		net_enable_timestamp();
> +
> +	return 0;
> +}
> +
> +late_initcall(net_dev_late_init);
>
Marcelo Tosatti Feb. 22, 2024, 5:52 p.m. UTC | #3 
On Thu, Feb 22, 2024 at 10:11:08AM -0500, Willem de Bruijn wrote:
> Marcelo Tosatti wrote:
> > For systems that use CPU isolation (via nohz_full), creating or destroying
> > a socket with  timestamping (SOF_TIMESTAMPING_OPT_TX_SWHW) might cause a
> > static key to be enabled/disabled. This in turn causes undesired 
> > IPIs to isolated CPUs.
> 
> This refers to SOF_TIMESTAMPING_RX_SOFTWARE, not SOF_TIMESTAMPING_OPT_TX_SWHW.
> See also sock_set_timestamping.

Willem,

This test program does trigger the static key change:

int main (void)
{
        int option = SOF_TIMESTAMPING_OPT_TX_SWHW;
        int sock_fd;
        int ret;
        int pid_fd;
        pid_t pid;
        char buf[50];

..

        /* set the timestamping option
         * this is to trigger the IPIs that notify all cpus of the change
         */
        if (setsockopt(sock_fd, SOL_SOCKET, SO_TIMESTAMP, &option, sizeof (option)) < 0) {
                printf("Could not enable timestamping option %x", (unsigned int)option);
                close(sock_fd);
                return 0;
        }
..
Willem de Bruijn Feb. 22, 2024, 6:42 p.m. UTC | #4 
Marcelo Tosatti wrote:
> On Thu, Feb 22, 2024 at 10:11:08AM -0500, Willem de Bruijn wrote:
> > Marcelo Tosatti wrote:
> > > For systems that use CPU isolation (via nohz_full), creating or destroying
> > > a socket with  timestamping (SOF_TIMESTAMPING_OPT_TX_SWHW) might cause a
> > > static key to be enabled/disabled. This in turn causes undesired 
> > > IPIs to isolated CPUs.
> > 
> > This refers to SOF_TIMESTAMPING_RX_SOFTWARE, not SOF_TIMESTAMPING_OPT_TX_SWHW.
> > See also sock_set_timestamping.
> 
> Willem,
> 
> This test program does trigger the static key change:
> 
> int main (void)
> {
>         int option = SOF_TIMESTAMPING_OPT_TX_SWHW;
>         int sock_fd;
>         int ret;
>         int pid_fd;
>         pid_t pid;
>         char buf[50];
> 
> ...
> 
>         /* set the timestamping option
>          * this is to trigger the IPIs that notify all cpus of the change
>          */
>         if (setsockopt(sock_fd, SOL_SOCKET, SO_TIMESTAMP, &option, sizeof (option)) < 0) {
>                 printf("Could not enable timestamping option %x", (unsigned int)option);
>                 close(sock_fd);
>                 return 0;
>         }
> ...
> 

That is because you call SO_TIMESTAMP, which interprets option as a
boolean. The SOF_ flags apply to setsockopt SO_TIMESTAMPING.
diff mbox series

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 0d548431f3fa..cc9a77b4aa4e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -153,6 +153,7 @@ 
 #include <linux/prandom.h>
 #include <linux/once_lite.h>
 #include <net/netdev_rx_queue.h>
+#include <linux/sched/isolation.h>
 
 #include "dev.h"
 #include "net-sysfs.h"
@@ -11601,3 +11602,14 @@  static int __init net_dev_init(void)
 }
 
 subsys_initcall(net_dev_init);
+
+static int __init net_dev_late_init(void)
+{
+	/* avoid static key IPIs to isolated CPUs */
+	if (housekeeping_enabled(HK_TYPE_MISC))
+		net_enable_timestamp();
+
+	return 0;
+}
+
+late_initcall(net_dev_late_init);