Performance regression in ip_set_swap on 6.1.69

Performance regression in ip_set_swap on 6.1.69

[Ale Crismani]

Dear all,

When upgrading some of our Debian hosts that compose a Kubernetes cluster we found a regression in ip_set_swap on 6.1.69. Calls to ip_set_swap now take roughly 15ms, while they used to take just tens of microseconds before.

The issue is very visible for use, since we use kube-router as our Kubernetes networking interface, and it uses ipset swap all the time to populate sets that enforce firewall policies between containers.

We tracked the issue down with strace, and then took stats with bpftrace running:
---
kfunc:ip_set:ip_set_swap {
        @start[tid] = nsecs;
}

kretfunc:ip_set:ip_set_swap {
        if (@start[tid]) {
                @srlat = hist((nsecs - @start[tid])/1000);
                delete(@start[tid]);
        }
}

interval:s:20 {
        printf("ip_set_swap() latency, milliseconds:\n");
---

On 6.1.69 results look like:
 
ip_set_swap() latency, milliseconds:
[8K, 16K)           1848 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[16K, 32K)         1017 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[32K, 64K)         38     |@

while on 6.1.67:
ip_set_swap() latency, milliseconds:

[0]                  166   |@
[1]                  378   |@@
[2, 4)              762   |@@@@@
[4, 8)              1624 |@@@@@@@@@@@
[8, 16)            3493 |@@@@@@@@@@@@@@@@@@@@@@@@
[16, 32)          7308 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[32, 64)          6412 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[64, 128)              1 |

We tried compiling commits between 6.1.67 and 6.1.69 and it seems the performance regression was introduced by 875ee3a, ip_set_swap is fast on 602505 that precedes it, and slow on it.

First time I post here, hope the format is appropriate, and thanks for any help with this! Also, if possible, I'd appreciate if any reply could CC me, as I am not subscribed.

Alessandro Crismani

Re: Performance regression in ip_set_swap on 6.1.69

[David Wang]

I confirmed this on 6.7 that this was introduced by commit 28628fa952fefc7f2072ce6e8016968cc452b1ba with following changes:

	 static inline void
	@@ -1397,6 +1394,9 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info,
		ip_set(inst, to_id) = from;
		write_unlock_bh(&ip_set_ref_lock);
	 
	+       /* Make sure all readers of the old set pointers are completed. */
	+       synchronize_rcu();
	+
		return 0;
	 }

synchronize_rcu causes the delay, and its usage here is very confusing, there is no reclaimer code after it.


FYI
David



 

Re: Performance regression in ip_set_swap on 6.1.69

[Jozsef Kadlecsik]

On Wed, 10 Jan 2024, David Wang wrote:

> I confirmed this on 6.7 that this was introduced by commit 
> 28628fa952fefc7f2072ce6e8016968cc452b1ba with following changes:
> 
> 	 static inline void
> 	@@ -1397,6 +1394,9 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info,
> 		ip_set(inst, to_id) = from;
> 		write_unlock_bh(&ip_set_ref_lock);
> 	 
> 	+       /* Make sure all readers of the old set pointers are completed. */
> 	+       synchronize_rcu();
> 	+
> 		return 0;
> 	 }
> 
> synchronize_rcu causes the delay, and its usage here is very confusing, 
> there is no reclaimer code after it.

As I'm seeing just the end of the discussion, please send a full report of 
the problem and how to reproduce it.

Best regards,
Jozsef
-- 
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: Performance regression in ip_set_swap on 6.1.69

[David Wang]

At 2024-01-10 18:35:02, "Jozsef Kadlecsik" <kadlec@netfilter.org> wrote:
>On Wed, 10 Jan 2024, David Wang wrote:
>
>> I confirmed this on 6.7 that this was introduced by commit 
>> 28628fa952fefc7f2072ce6e8016968cc452b1ba with following changes:
>> 
>> 	 static inline void
>> 	@@ -1397,6 +1394,9 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info,
>> 		ip_set(inst, to_id) = from;
>> 		write_unlock_bh(&ip_set_ref_lock);
>> 	 
>> 	+       /* Make sure all readers of the old set pointers are completed. */
>> 	+       synchronize_rcu();
>> 	+
>> 		return 0;
>> 	 }
>> 
>> synchronize_rcu causes the delay, and its usage here is very confusing, 
>> there is no reclaimer code after it.
>
>As I'm seeing just the end of the discussion, please send a full report of 
>the problem and how to reproduce it.
>

This was reported in https://lore.kernel.org/lkml/C0829B10-EAA6-4809-874E-E1E9C05A8D84@automattic.com/ by ale.crismani@automattic.com
Just out of interest of performance issues,  I tried to reproduce it with a test stressing ipset_swap:

My test code is as following, it would stress swapping ipset 'foo' with 'bar'; (foo/bar ipset needs to be created before the test.)
With latest 6.7, the stress would take about 180 seconds to finish, but with `synchronize_rcu` removed, it only took 3seconds.


```
unsigned char mbuffer[4096];
int main() {
	int err;
	int sock = socket(AF_NETLINK, SOCK_RAW, NETLINK_NETFILTER);
	if (sock<0) {
		perror("Fail to create socket");
		return 1;
	}
	struct sockaddr_nl addr = {
		.nl_family = AF_NETLINK,
		.nl_pad = 0,
		.nl_pid = 0,
		.nl_groups = 0
	};
	struct sockaddr raddr = {0};
	socklen_t rsize;
	int seq = 0x12345678;
	err = bind(sock, (struct sockaddr*)&addr, sizeof(addr));
	if (err) {
		perror("Fail to bind");
		return 1;
	}
	err = getsockname(sock, &raddr, &rsize);
	if (err) {
		perror("Fail to getsockname");
		return 1;
	}
	unsigned char buf[64];
	struct nlmsghdr *phdr;
	struct nfgenmsg *pnfg;
	struct nlattr *pnla;
	unsigned int total;
	ssize_t rz;
	struct iovec iovs;
	iovs.iov_base = mbuffer;
	iovs.iov_len = sizeof(mbuffer);
	struct msghdr msg = {0};
	msg.msg_name = &addr;
	msg.msg_namelen = sizeof(addr);
	msg.msg_iov = &iovs;
	msg.msg_iovlen = 1;

	memset(buf, 0, sizeof(buf));
	total = 0;
	phdr = (struct nlmsghdr*)(buf+total);
	total += sizeof(struct nlmsghdr);
	phdr->nlmsg_type=NFNL_SUBSYS_IPSET<<8|IPSET_CMD_PROTOCOL;
	phdr->nlmsg_seq = seq;
	phdr->nlmsg_flags = NLM_F_REQUEST;
	pnfg = (struct nfgenmsg*)(buf+total);
	total += sizeof(struct nfgenmsg);
	pnfg->nfgen_family=AF_INET;
    	pnfg->version= NFNETLINK_V0;
	pnfg->res_id=htons(0);
	pnla = (struct nlattr *)(buf+total);
	pnla->nla_len = 5;
	pnla->nla_type = 1;
	buf[total+sizeof(struct nlattr)]=0x06;
	total+=8;
	phdr->nlmsg_len = total;
	rz = sendto(sock, buf, total, 0, (struct sockaddr*)&addr, sizeof(addr));
	rz = recvmsg(sock, &msg, 0);

	pnla = (struct nlattr *)(buf+total);
	pnla->nla_len = 8;
	pnla->nla_type = 2;
	char *p = buf+(total+sizeof(struct nlattr));
	p[0]='f'; p[1]='o'; p[2]='o'; p[3]=0;
	total+=8;
	pnla = (struct nlattr *)(buf+total);
	pnla->nla_len = 8;
	pnla->nla_type = 3;
	p = buf+(total+sizeof(struct nlattr));
	p[0]='b'; p[1]='a'; p[2]='r'; p[3]=0;
	total+=8;
	phdr->nlmsg_type = NFNL_SUBSYS_IPSET<<8|IPSET_CMD_SWAP;
	phdr->nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
	phdr->nlmsg_len = total;

	
	for (int i=0; i<10000; i++) {
		// stress swap foo bar
		phdr->nlmsg_seq++;
		sendto(sock, buf, total, 0, (struct sockaddr*)&addr, sizeof(addr));
		recvmsg(sock, &msg, 0);
	}

	close(sock);
	return 0;
}
```

>Best regards,
>Jozsef

David

Re: Performance regression in ip_set_swap on 6.1.69

[David Wang]

At 2024-01-10 19:05:18, "David Wang" <00107082@163.com> wrote:
>
>
>My test code is as following, it would stress swapping ipset 'foo' with 'bar'; (foo/bar ipset needs to be created before the test.)
>With latest 6.7, the stress would take about 180 seconds to finish, but with `synchronize_rcu` removed, it only took 3seconds.

Correction,  with 'synchronize_rcu' removed, it only took about 0.03 seconds
>
>
>David

Re: Performance regression in ip_set_swap on 6.1.69

[Jozsef Kadlecsik]

Hi,

On Wed, 10 Jan 2024, David Wang wrote:

> At 2024-01-10 18:35:02, "Jozsef Kadlecsik" <kadlec@netfilter.org> wrote:
> >On Wed, 10 Jan 2024, David Wang wrote:
> >
> >> I confirmed this on 6.7 that this was introduced by commit 
> >> 28628fa952fefc7f2072ce6e8016968cc452b1ba with following changes:
> >> 
> >> 	 static inline void
> >> 	@@ -1397,6 +1394,9 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info,
> >> 		ip_set(inst, to_id) = from;
> >> 		write_unlock_bh(&ip_set_ref_lock);
> >> 	 
> >> 	+       /* Make sure all readers of the old set pointers are completed. */
> >> 	+       synchronize_rcu();
> >> 	+
> >> 		return 0;
> >> 	 }
> >> 
> >> synchronize_rcu causes the delay, and its usage here is very confusing, 
> >> there is no reclaimer code after it.
> >
> >As I'm seeing just the end of the discussion, please send a full report of 
> >the problem and how to reproduce it.
> >
> 
> This was reported in 
> https://lore.kernel.org/lkml/C0829B10-EAA6-4809-874E-E1E9C05A8D84@automattic.com/ 
> by ale.crismani@automattic.com Just out of interest of performance 
> issues, I tried to reproduce it with a test stressing ipset_swap:
> 
> My test code is as following, it would stress swapping ipset 'foo' with 
> 'bar'; (foo/bar ipset needs to be created before the test.) With latest 
> 6.7, the stress would take about 180 seconds to finish, but with 
> `synchronize_rcu` removed, it only took 3seconds.
> 
> 
> ```
> unsigned char mbuffer[4096];
> int main() {
> 	int err;
> 	int sock = socket(AF_NETLINK, SOCK_RAW, NETLINK_NETFILTER);
> 	if (sock<0) {
> 		perror("Fail to create socket");
> 		return 1;
> 	}
> 	struct sockaddr_nl addr = {
> 		.nl_family = AF_NETLINK,
> 		.nl_pad = 0,
> 		.nl_pid = 0,
> 		.nl_groups = 0
> 	};
> 	struct sockaddr raddr = {0};
> 	socklen_t rsize;
> 	int seq = 0x12345678;
> 	err = bind(sock, (struct sockaddr*)&addr, sizeof(addr));
> 	if (err) {
> 		perror("Fail to bind");
> 		return 1;
> 	}
> 	err = getsockname(sock, &raddr, &rsize);
> 	if (err) {
> 		perror("Fail to getsockname");
> 		return 1;
> 	}
> 	unsigned char buf[64];
> 	struct nlmsghdr *phdr;
> 	struct nfgenmsg *pnfg;
> 	struct nlattr *pnla;
> 	unsigned int total;
> 	ssize_t rz;
> 	struct iovec iovs;
> 	iovs.iov_base = mbuffer;
> 	iovs.iov_len = sizeof(mbuffer);
> 	struct msghdr msg = {0};
> 	msg.msg_name = &addr;
> 	msg.msg_namelen = sizeof(addr);
> 	msg.msg_iov = &iovs;
> 	msg.msg_iovlen = 1;
> 
> 	memset(buf, 0, sizeof(buf));
> 	total = 0;
> 	phdr = (struct nlmsghdr*)(buf+total);
> 	total += sizeof(struct nlmsghdr);
> 	phdr->nlmsg_type=NFNL_SUBSYS_IPSET<<8|IPSET_CMD_PROTOCOL;
> 	phdr->nlmsg_seq = seq;
> 	phdr->nlmsg_flags = NLM_F_REQUEST;
> 	pnfg = (struct nfgenmsg*)(buf+total);
> 	total += sizeof(struct nfgenmsg);
> 	pnfg->nfgen_family=AF_INET;
>     	pnfg->version= NFNETLINK_V0;
> 	pnfg->res_id=htons(0);
> 	pnla = (struct nlattr *)(buf+total);
> 	pnla->nla_len = 5;
> 	pnla->nla_type = 1;
> 	buf[total+sizeof(struct nlattr)]=0x06;
> 	total+=8;
> 	phdr->nlmsg_len = total;
> 	rz = sendto(sock, buf, total, 0, (struct sockaddr*)&addr, sizeof(addr));
> 	rz = recvmsg(sock, &msg, 0);
> 
> 	pnla = (struct nlattr *)(buf+total);
> 	pnla->nla_len = 8;
> 	pnla->nla_type = 2;
> 	char *p = buf+(total+sizeof(struct nlattr));
> 	p[0]='f'; p[1]='o'; p[2]='o'; p[3]=0;
> 	total+=8;
> 	pnla = (struct nlattr *)(buf+total);
> 	pnla->nla_len = 8;
> 	pnla->nla_type = 3;
> 	p = buf+(total+sizeof(struct nlattr));
> 	p[0]='b'; p[1]='a'; p[2]='r'; p[3]=0;
> 	total+=8;
> 	phdr->nlmsg_type = NFNL_SUBSYS_IPSET<<8|IPSET_CMD_SWAP;
> 	phdr->nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
> 	phdr->nlmsg_len = total;
> 
> 	
> 	for (int i=0; i<10000; i++) {
> 		// stress swap foo bar
> 		phdr->nlmsg_seq++;
> 		sendto(sock, buf, total, 0, (struct sockaddr*)&addr, sizeof(addr));
> 		recvmsg(sock, &msg, 0);
> 	}
> 
> 	close(sock);
> 	return 0;
> }
> ```

Thanks, I'll look into it. The race condition fix between swap/destroy and 
kernel side add/del/test had several versions, either penalizing destroy 
or swap. Finally swap seemed to be the less intrusive. I'm going to 
explore other possibilities.

Best regards,
Jozsef
-- 
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: Performance regression in ip_set_swap on 6.1.69

[Jozsef Kadlecsik]

Hi,

On Wed, 10 Jan 2024, Jozsef Kadlecsik wrote:

> On Wed, 10 Jan 2024, David Wang wrote:
> 
> > At 2024-01-10 18:35:02, "Jozsef Kadlecsik" <kadlec@netfilter.org> wrote:
> > >On Wed, 10 Jan 2024, David Wang wrote:
> > >
> > >> I confirmed this on 6.7 that this was introduced by commit 
> > >> 28628fa952fefc7f2072ce6e8016968cc452b1ba with following changes:
> > >> 
> > >> 	 static inline void
> > >> 	@@ -1397,6 +1394,9 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info,
> > >> 		ip_set(inst, to_id) = from;
> > >> 		write_unlock_bh(&ip_set_ref_lock);
> > >> 	 
> > >> 	+       /* Make sure all readers of the old set pointers are completed. */
> > >> 	+       synchronize_rcu();
> > >> 	+
> > >> 		return 0;
> > >> 	 }
> > >> 
> > >> synchronize_rcu causes the delay, and its usage here is very confusing, 
> > >> there is no reclaimer code after it.
> > >
> > >As I'm seeing just the end of the discussion, please send a full report of 
> > >the problem and how to reproduce it.
> > >
> > 
> > This was reported in 
> > https://lore.kernel.org/lkml/C0829B10-EAA6-4809-874E-E1E9C05A8D84@automattic.com/ 
> > by ale.crismani@automattic.com Just out of interest of performance 
> > issues, I tried to reproduce it with a test stressing ipset_swap:
> > 
> > My test code is as following, it would stress swapping ipset 'foo' with 
> > 'bar'; (foo/bar ipset needs to be created before the test.) With latest 
> > 6.7, the stress would take about 180 seconds to finish, but with 
> > `synchronize_rcu` removed, it only took 3seconds.
> > 
> > 
> > ```
> > unsigned char mbuffer[4096];
> > int main() {
> > 	int err;
> > 	int sock = socket(AF_NETLINK, SOCK_RAW, NETLINK_NETFILTER);
> > 	if (sock<0) {
> > 		perror("Fail to create socket");
> > 		return 1;
> > 	}
> > 	struct sockaddr_nl addr = {
> > 		.nl_family = AF_NETLINK,
> > 		.nl_pad = 0,
> > 		.nl_pid = 0,
> > 		.nl_groups = 0
> > 	};
> > 	struct sockaddr raddr = {0};
> > 	socklen_t rsize;
> > 	int seq = 0x12345678;
> > 	err = bind(sock, (struct sockaddr*)&addr, sizeof(addr));
> > 	if (err) {
> > 		perror("Fail to bind");
> > 		return 1;
> > 	}
> > 	err = getsockname(sock, &raddr, &rsize);
> > 	if (err) {
> > 		perror("Fail to getsockname");
> > 		return 1;
> > 	}
> > 	unsigned char buf[64];
> > 	struct nlmsghdr *phdr;
> > 	struct nfgenmsg *pnfg;
> > 	struct nlattr *pnla;
> > 	unsigned int total;
> > 	ssize_t rz;
> > 	struct iovec iovs;
> > 	iovs.iov_base = mbuffer;
> > 	iovs.iov_len = sizeof(mbuffer);
> > 	struct msghdr msg = {0};
> > 	msg.msg_name = &addr;
> > 	msg.msg_namelen = sizeof(addr);
> > 	msg.msg_iov = &iovs;
> > 	msg.msg_iovlen = 1;
> > 
> > 	memset(buf, 0, sizeof(buf));
> > 	total = 0;
> > 	phdr = (struct nlmsghdr*)(buf+total);
> > 	total += sizeof(struct nlmsghdr);
> > 	phdr->nlmsg_type=NFNL_SUBSYS_IPSET<<8|IPSET_CMD_PROTOCOL;
> > 	phdr->nlmsg_seq = seq;
> > 	phdr->nlmsg_flags = NLM_F_REQUEST;
> > 	pnfg = (struct nfgenmsg*)(buf+total);
> > 	total += sizeof(struct nfgenmsg);
> > 	pnfg->nfgen_family=AF_INET;
> >     	pnfg->version= NFNETLINK_V0;
> > 	pnfg->res_id=htons(0);
> > 	pnla = (struct nlattr *)(buf+total);
> > 	pnla->nla_len = 5;
> > 	pnla->nla_type = 1;
> > 	buf[total+sizeof(struct nlattr)]=0x06;
> > 	total+=8;
> > 	phdr->nlmsg_len = total;
> > 	rz = sendto(sock, buf, total, 0, (struct sockaddr*)&addr, sizeof(addr));
> > 	rz = recvmsg(sock, &msg, 0);
> > 
> > 	pnla = (struct nlattr *)(buf+total);
> > 	pnla->nla_len = 8;
> > 	pnla->nla_type = 2;
> > 	char *p = buf+(total+sizeof(struct nlattr));
> > 	p[0]='f'; p[1]='o'; p[2]='o'; p[3]=0;
> > 	total+=8;
> > 	pnla = (struct nlattr *)(buf+total);
> > 	pnla->nla_len = 8;
> > 	pnla->nla_type = 3;
> > 	p = buf+(total+sizeof(struct nlattr));
> > 	p[0]='b'; p[1]='a'; p[2]='r'; p[3]=0;
> > 	total+=8;
> > 	phdr->nlmsg_type = NFNL_SUBSYS_IPSET<<8|IPSET_CMD_SWAP;
> > 	phdr->nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
> > 	phdr->nlmsg_len = total;
> > 
> > 	
> > 	for (int i=0; i<10000; i++) {
> > 		// stress swap foo bar
> > 		phdr->nlmsg_seq++;
> > 		sendto(sock, buf, total, 0, (struct sockaddr*)&addr, sizeof(addr));
> > 		recvmsg(sock, &msg, 0);
> > 	}
> > 
> > 	close(sock);
> > 	return 0;
> > }
> > ```
> 
> Thanks, I'll look into it. The race condition fix between swap/destroy and 
> kernel side add/del/test had several versions, either penalizing destroy 
> or swap. Finally swap seemed to be the less intrusive. I'm going to 
> explore other possibilities.

Could you check that the patch below fixes the performance regression? 
Instead of waiting for the RCU grace period at swapping, call_rcu() is 
used at destroying the set.

diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
index e8c350a3ade1..912f750d0bea 100644
--- a/include/linux/netfilter/ipset/ip_set.h
+++ b/include/linux/netfilter/ipset/ip_set.h
@@ -242,6 +242,8 @@ extern void ip_set_type_unregister(struct ip_set_type *set_type);
 
 /* A generic IP set */
 struct ip_set {
+	/* For call_cru in destroy */
+	struct rcu_head rcu;
 	/* The name of the set */
 	char name[IPSET_MAXNAMELEN];
 	/* Lock protecting the set data */
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 4c133e06be1d..cd95f75dd720 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1009,6 +1009,14 @@ find_set(struct ip_set_net *inst, const char *name)
 	return find_set_and_id(inst, name, &id);
 }
 
+static void
+ip_set_destroy_set_rcu(struct rcu_head *head)
+{
+	struct ip_set *set = container_of(head, struct ip_set, rcu);
+
+	ip_set_destroy_set(set);
+}
+
 static int
 find_free_id(struct ip_set_net *inst, const char *name, ip_set_id_t *index,
 	     struct ip_set **set)
@@ -1241,7 +1249,7 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 		ip_set(inst, i) = NULL;
 		read_unlock_bh(&ip_set_ref_lock);
 
-		ip_set_destroy_set(s);
+		call_rcu(&s->rcu, ip_set_destroy_set_rcu);
 	}
 	return 0;
 out:
@@ -1394,9 +1402,6 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info,
 	ip_set(inst, to_id) = from;
 	write_unlock_bh(&ip_set_ref_lock);
 
-	/* Make sure all readers of the old set pointers are completed. */
-	synchronize_rcu();
-
 	return 0;
 }
 

Best regards,
Jozsef
-- 
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re:Re: Performance regression in ip_set_swap on 6.1.69

[David Wang]

At 2024-01-11 16:25:46, "Jozsef Kadlecsik" <kadlec@blackhole.kfki.hu> wrote:
>Hi,
>
>
>Could you check that the patch below fixes the performance regression? 
>Instead of waiting for the RCU grace period at swapping, call_rcu() is 
>used at destroying the set.

Got a compiler error:
net/netfilter/ipset/ip_set_core.c: In function ‘ip_set_destroy_set_rcu’:
net/netfilter/ipset/ip_set_core.c:1017:9: error: implicit declaration of function ‘ip_set_destroy_set’; did you mean ‘ip_set_destroy_set_rcu’? [-Werror=implicit-function-declaration]
 1017 |         ip_set_destroy_set(set);
      |         ^~~~~~~~~~~~~~~~~~
      |         ip_set_destroy_set_rcu
net/netfilter/ipset/ip_set_core.c: At top level:
net/netfilter/ipset/ip_set_core.c:1183:1: warning: conflicting types for ‘ip_set_destroy_set’; have ‘void(struct ip_set *)’
 1183 | ip_set_destroy_set(struct ip_set *set)
      | ^~~~~~~~~~~~~~~~~~
net/netfilter/ipset/ip_set_core.c:1183:1: error: static declaration of ‘ip_set_destroy_set’ follows non-static declaration


I move the declaration of ip_set_destroy_set_rcu,  make sure it is after the declaration of ip_set_destroy_set, 
With this path, the performance degradation of ipset_swap is gone,  but my test only stress ipset_swap, not a swap/destroy sequence.
I will adjust my  code to stress a full swap/destroy/create/add sequence, and update later.


Thanks
David

Re:Re: Performance regression in ip_set_swap on 6.1.69

[Jozsef Kadlecsik]

[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]

Hi,

On Thu, 11 Jan 2024, David Wang wrote:

> At 2024-01-11 16:25:46, "Jozsef Kadlecsik" <kadlec@blackhole.kfki.hu> wrote:
> >
> >
> >Could you check that the patch below fixes the performance regression? 
> >Instead of waiting for the RCU grace period at swapping, call_rcu() is 
> >used at destroying the set.
> 
> Got a compiler error:
> net/netfilter/ipset/ip_set_core.c: In function ‘ip_set_destroy_set_rcu’:
> net/netfilter/ipset/ip_set_core.c:1017:9: error: implicit declaration of function ‘ip_set_destroy_set’; did you mean ‘ip_set_destroy_set_rcu’? [-Werror=implicit-function-declaration]
>  1017 |         ip_set_destroy_set(set);
>       |         ^~~~~~~~~~~~~~~~~~
>       |         ip_set_destroy_set_rcu
> net/netfilter/ipset/ip_set_core.c: At top level:
> net/netfilter/ipset/ip_set_core.c:1183:1: warning: conflicting types for ‘ip_set_destroy_set’; have ‘void(struct ip_set *)’
>  1183 | ip_set_destroy_set(struct ip_set *set)
>       | ^~~~~~~~~~~~~~~~~~
> net/netfilter/ipset/ip_set_core.c:1183:1: error: static declaration of ‘ip_set_destroy_set’ follows non-static declaration
>
>  I move the declaration of ip_set_destroy_set_rcu, make sure it is after 
> the declaration of ip_set_destroy_set, With this path, the performance 
> degradation of ipset_swap is gone,

Thanks! I wrote the patch for the ipset package code and while it applies 
to the vanilla kernel source, function definiton ordering got broken. 
Sorry, I should have to double check.

> but my test only stress ipset_swap, not a swap/destroy sequence. I will 
> adjust my code to stress a full swap/destroy/create/add sequence, and 
> update later.

The synchronize_rcu() in ip_set_swap() was added to exclude the parallel 
swap+destroy and kernel side add/del/test operations, which can be tested 
with the commands

    ipset create hash_ip1 hash:net family inet hashsize 1024 maxelem 1048576
    ipset add hash_ip1 172.20.0.0/16
    ipset add hash_ip1 192.168.0.0/16
    iptables -A INPUT -m set --match-set hash_ip1 src -j ACCEPT
    while [ 1 ]
    do
            # ... Ongoing traffic...
            ipset create hash_ip2 hash:net family inet hashsize 1024 maxelem 1048576
            ipset add hash_ip2 172.20.0.0/16
            ipset swap hash_ip1 hash_ip2
            ipset destroy hash_ip2
            sleep 0.05
    done

I have checked it but the virtual machine on my dev laptop might be too 
slow to trigger the case.

Best regards,
Jozsef
-- 
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re:Performance regression in ip_set_swap on 6.7.0

[David Wang]

I tested the patch with code stressing swap->destroy->create->add 10000 times, the performance regression still happens, and now it is ip_set_destroy.
(I pasted the test code at the end of this mail)  
time show that most delay is 'off cpu':
	$ time sudo ./stressipset 

	real	2m45.115s
	user	0m0.019s
	sys	0m0.744s

Most time, callstack stuck in rcu_barrier:
	$ sudo cat /proc/2158/stack
	[<0>] rcu_barrier+0x1f6/0x2d0
	[<0>] ip_set_destroy+0x84/0x1d0 [ip_set]
	[<0>] nfnetlink_rcv_msg+0x2ac/0x2f0 [nfnetlink]
	[<0>] netlink_rcv_skb+0x57/0x100
	[<0>] netlink_unicast+0x19a/0x280
	[<0>] netlink_sendmsg+0x250/0x4d0
	[<0>] __sys_sendto+0x1be/0x1d0
	[<0>] __x64_sys_sendto+0x20/0x30
	[<0>] do_syscall_64+0x42/0xf0
	[<0>] entry_SYSCALL_64_after_hwframe+0x6e/0x76

perf_event_open profiling show similiar call signature for rcu_call and synchronize_rcu

	ip_set_destroy(49.651% 2133/4296)
	    rcu_barrier(80.684% 1721/2133)
		wait_for_completion(79.198% 1363/1721)
		    schedule_timeout(94.864% 1293/1363)
			schedule(96.520% 1248/1293)
			    __schedule(97.436% 1216/1248)
			    preempt_count_add(0.240% 3/1248)
			    srso_return_thunk(0.160% 2/1248)
			    preempt_count_sub(0.160% 2/1248)
			srso_return_thunk(0.077% 1/1293)
		    _raw_spin_unlock_irq(1.027% 14/1363)
		    _raw_spin_lock_irq(0.514% 7/1363)
		    __cond_resched(0.220% 3/1363)
		    srso_return_thunk(0.147% 2/1363)

	ip_set_swap(79.842% 709/888)  (this profiling was captured when synchronize_rcu is used in ip_set_swap)
	    synchronize_rcu(74.330% 527/709)
		__wait_rcu_gp(89.184% 470/527)
		    wait_for_completion(86.383% 406/470)
			schedule_timeout(91.133% 370/406)
			    schedule(95.135% 352/370)
			_raw_spin_unlock_irq(3.202% 13/406)
			_raw_spin_lock_irq(0.739% 3/406)
			srso_return_thunk(0.246% 1/406)
		    _raw_spin_unlock_irq(7.021% 33/470)
		    __call_rcu_common.constprop.0(3.830% 18/470)
		rcu_gp_is_expedited(3.036% 16/527)
		__cond_resched(0.569% 3/527)
		srso_return_thunk(0.190% 1/527)

They all call wait_for_completion, which may sleep on something on purpose, I guess...(Maybe rcu is not a good choice here?)


I made another test with 'synchronize_rcu' removed from ip_set_swap and no 'call_rcu' in ip_set_destroy, the performance is much better:
	$ time sudo ./stressipset 

	real	0m2.203s
	user	0m0.037s
	sys	0m0.188s


Here is my test code, it is very ugly...(I have no knowledge of netfilter messaging protocol, and just hack it out with the help of strace.)
And I was lazy, before run the test, foo/bar netset is needed:

	ipset create foo hash:net
	ipset create bar hash:net

Here come the ugly code, should be able to compile with gcc.

	#include <stdio.h>
	#include <stdlib.h>
	#include <sys/types.h>
	#include <sys/socket.h>
	#include <unistd.h>
	#include <arpa/inet.h>
	#include <linux/netlink.h>
	#include <linux/netfilter/nfnetlink.h>
	#include <linux/netfilter/ipset/ip_set.h>
	#include <string.h>

	unsigned char mbuffer[4096];
	unsigned char buf[128]; // cmd buffer  for swap
	unsigned char dbuf[128]; // cmd buffer for destroy
	unsigned char cbuf[128]; // cmd buffer for create
	unsigned char abuf[128]; // cmd buffer for add
	int main() {
		int err;
		int sock = socket(AF_NETLINK, SOCK_RAW, NETLINK_NETFILTER);
		if (sock<0) {
			perror("Fail to create socket");
			return 1;
		}
		struct sockaddr_nl addr = {
			.nl_family = AF_NETLINK,
			.nl_pad = 0,
			.nl_pid = 0,
			.nl_groups = 0
		};
		struct sockaddr raddr = {0};
		socklen_t rsize;
		int seq = 0x12345678;
		err = bind(sock, (struct sockaddr*)&addr, sizeof(addr));
		if (err) {
			perror("Fail to bind");
			return 1;
		}
		err = getsockname(sock, &raddr, &rsize);
		if (err) {
			perror("Fail to getsockname");
			return 1;
		}
		struct nlmsghdr *phdr;
		struct nfgenmsg *pnfg;
		struct nlattr *pnla;
		unsigned int total;
		ssize_t rz;
		struct iovec iovs;
		iovs.iov_base = mbuffer;
		iovs.iov_len = sizeof(mbuffer);
		struct msghdr msg = {0};
		msg.msg_name = &addr;
		msg.msg_namelen = sizeof(addr);
		msg.msg_iov = &iovs;
		msg.msg_iovlen = 1;

		memset(buf, 0, sizeof(buf));
		total = 0;
		phdr = (struct nlmsghdr*)(buf+total);
		total += sizeof(struct nlmsghdr);
		phdr->nlmsg_type=NFNL_SUBSYS_IPSET<<8|IPSET_CMD_PROTOCOL;
		phdr->nlmsg_seq = seq;
		phdr->nlmsg_flags = NLM_F_REQUEST;
		pnfg = (struct nfgenmsg*)(buf+total);
		total += sizeof(struct nfgenmsg);
		pnfg->nfgen_family=AF_INET;
		pnfg->version= NFNETLINK_V0;
		pnfg->res_id=htons(0);
		pnla = (struct nlattr *)(buf+total);
		pnla->nla_len = 5;
		pnla->nla_type = 1;
		buf[total+sizeof(struct nlattr)]=0x06;
		total+=8;
		phdr->nlmsg_len = total;
		rz = sendto(sock, buf, total, 0, (struct sockaddr*)&addr, sizeof(addr));
		rz = recvmsg(sock, &msg, 0);

		int sz = total;
		// build swap
		pnla = (struct nlattr *)(buf+total);
		pnla->nla_len = 8;
		pnla->nla_type = 2;
		char *p = buf+(total+sizeof(struct nlattr));
		p[0]='f'; p[1]='o'; p[2]='o'; p[3]=0;
		total+=8;
		pnla = (struct nlattr *)(buf+total);
		pnla->nla_len = 8;
		pnla->nla_type = 3;
		p = buf+(total+sizeof(struct nlattr));
		p[0]='b'; p[1]='a'; p[2]='r'; p[3]=0;
		total+=8;
		phdr->nlmsg_type = NFNL_SUBSYS_IPSET<<8|IPSET_CMD_SWAP;
		phdr->nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
		phdr->nlmsg_len = total;

		// build destroy
		char bar[] = "\x62\x61\x72\x00";
		memcpy(dbuf, buf, sz);
		total = sz;
		struct nlmsghdr *phdr_d = (struct nlmsghdr*)dbuf;
		pnla = (struct nlattr *)(dbuf+total);
		pnla->nla_len = 8;
		pnla->nla_type = 2;
		memcpy(dbuf+(total+sizeof(struct nlattr)), bar, sizeof(bar));
		total+=pnla->nla_len;
		phdr_d->nlmsg_type = NFNL_SUBSYS_IPSET<<8|IPSET_CMD_DESTROY;
		phdr_d->nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
		phdr_d->nlmsg_len = total;

		// build create
		memcpy(cbuf, buf, sz);
		total = sz;
		struct nlmsghdr *phdr_c = (struct nlmsghdr*)cbuf;
		pnla = (struct nlattr *)(cbuf+total);
		pnla->nla_len = 8;
		pnla->nla_type = 2;
		memcpy(cbuf+(total+sizeof(struct nlattr)), bar, sizeof(bar));
		total+=(pnla->nla_len+3)/4*4;
		char hashnet[] = "\x68\x61\x73\x68\x3a\x6e\x65\x74\x00";
		pnla = (struct nlattr *)(cbuf+total);
		pnla->nla_len = 13;
		pnla->nla_type = 3;
		memcpy(cbuf+(total+sizeof(struct nlattr)), hashnet, sizeof(hashnet));
		total+=(pnla->nla_len+3)/4*4;
		pnla = (struct nlattr *)(cbuf+total);
		pnla->nla_len = 5;
		pnla->nla_type = 4;
		cbuf[total+sizeof(struct nlattr)]=6;
		total+=(pnla->nla_len+3)/4*4;
		pnla = (struct nlattr *)(cbuf+total);
		pnla->nla_len = 5;
		pnla->nla_type = 5;
		cbuf[total+sizeof(struct nlattr)]=2;
		total+=(pnla->nla_len+3)/4*4;
		pnla = (struct nlattr *)(cbuf+total);
		pnla->nla_len = 4;
		pnla->nla_type = NLA_F_NESTED|0x7;
		total+=(pnla->nla_len+3)/4*4;

		phdr_c->nlmsg_type = NFNL_SUBSYS_IPSET<<8|IPSET_CMD_CREATE;
		phdr_c->nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
		phdr_c->nlmsg_len = total;

		// build add 
		memcpy(abuf, buf, sz);
		total = sz;
		struct nlmsghdr *phdr_a = (struct nlmsghdr*)abuf;
		pnla = (struct nlattr *)(abuf+total);
		pnla->nla_len = 8;
		pnla->nla_type = 2;
		memcpy(abuf+(total+sizeof(struct nlattr)), bar, sizeof(bar));
		total+=(pnla->nla_len+3)/4*4;
		char ip[] = "\x0c\x00\x01\x80\x08\x00\x01\x40\x0a\x01\x00\x00\x05\x00\x03\x00\x10\x00\x00\x00\x08\x00\x09\x40\x00\x00\x00\x00";
		pnla = (struct nlattr *)(abuf+total);
		pnla->nla_len = 32;
		pnla->nla_type = NLA_F_NESTED|0x7;
		memcpy(abuf+(total+sizeof(struct nlattr)), ip, sizeof(ip));
		total+=(pnla->nla_len+3)/4*4;
		phdr_a->nlmsg_type = NFNL_SUBSYS_IPSET<<8|IPSET_CMD_ADD;
		phdr_a->nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
		phdr_a->nlmsg_len = total;

		
		for (int i=0; i<10000; i++) {
			// swap foo bar
			seq++;
			phdr->nlmsg_seq = seq;
			sendto(sock, buf, phdr->nlmsg_len, 0, (struct sockaddr*)&addr, sizeof(addr));
			recvmsg(sock, &msg, 0);
			err=*(short*)(mbuffer+(sizeof(struct nlmsghdr)-2)); if (err) { printf("fail to swap %d\n", err); break; }
			// destroy bar
			seq++;
			phdr_d->nlmsg_seq = seq;
			sendto(sock, dbuf, phdr_d->nlmsg_len, 0, (struct sockaddr*)&addr, sizeof(addr));
			recvmsg(sock, &msg, 0);
			err=*(short*)(mbuffer+(sizeof(struct nlmsghdr)-2)); if (err) { printf("fail to destroy\n"); break; }
			// create bar
			seq++;
			phdr_c->nlmsg_seq = seq;
			sendto(sock, cbuf, phdr_c->nlmsg_len, 0, (struct sockaddr*)&addr, sizeof(addr));
			recvmsg(sock, &msg, 0);
			err=*(short*)(mbuffer+(sizeof(struct nlmsghdr)-2)); if (err) { printf("fail to create\n"); break; }
			// add bar
			seq++;
			phdr_a->nlmsg_seq = seq;
			sendto(sock, abuf, phdr_a->nlmsg_len, 0, (struct sockaddr*)&addr, sizeof(addr));
			recvmsg(sock, &msg, 0);
			err=*(short*)(mbuffer+(sizeof(struct nlmsghdr)-2)); if (err) { printf("fail to add\n"); break; }
		}

		close(sock);
		return 0;
	}


Thanks
David

Re:Performance regression in ip_set_swap on 6.7.0

[Jozsef Kadlecsik]

On Thu, 11 Jan 2024, David Wang wrote:

> I tested the patch with code stressing swap->destroy->create->add 10000 
> times, the performance regression still happens, and now it is 
> ip_set_destroy. (I pasted the test code at the end of this mail)
> time show that most delay is 'off cpu':
> 	$ time sudo ./stressipset 
> 
> 	real	2m45.115s
> 	user	0m0.019s
> 	sys	0m0.744s
> 
> Most time, callstack stuck in rcu_barrier:
> 	$ sudo cat /proc/2158/stack
> 	[<0>] rcu_barrier+0x1f6/0x2d0
> 	[<0>] ip_set_destroy+0x84/0x1d0 [ip_set]
> 	[<0>] nfnetlink_rcv_msg+0x2ac/0x2f0 [nfnetlink]
> 	[<0>] netlink_rcv_skb+0x57/0x100
> 	[<0>] netlink_unicast+0x19a/0x280
> 	[<0>] netlink_sendmsg+0x250/0x4d0
> 	[<0>] __sys_sendto+0x1be/0x1d0
> 	[<0>] __x64_sys_sendto+0x20/0x30
> 	[<0>] do_syscall_64+0x42/0xf0
> 	[<0>] entry_SYSCALL_64_after_hwframe+0x6e/0x76
> 
> perf_event_open profiling show similiar call signature for rcu_call and synchronize_rcu
> 
> 	ip_set_destroy(49.651% 2133/4296)
> 	    rcu_barrier(80.684% 1721/2133)
> 		wait_for_completion(79.198% 1363/1721)
> 		    schedule_timeout(94.864% 1293/1363)
> 			schedule(96.520% 1248/1293)
> 			    __schedule(97.436% 1216/1248)
> 			    preempt_count_add(0.240% 3/1248)
> 			    srso_return_thunk(0.160% 2/1248)
> 			    preempt_count_sub(0.160% 2/1248)
> 			srso_return_thunk(0.077% 1/1293)
> 		    _raw_spin_unlock_irq(1.027% 14/1363)
> 		    _raw_spin_lock_irq(0.514% 7/1363)
> 		    __cond_resched(0.220% 3/1363)
> 		    srso_return_thunk(0.147% 2/1363)
> 
> 	ip_set_swap(79.842% 709/888)  (this profiling was captured when synchronize_rcu is used in ip_set_swap)
> 	    synchronize_rcu(74.330% 527/709)
> 		__wait_rcu_gp(89.184% 470/527)
> 		    wait_for_completion(86.383% 406/470)
> 			schedule_timeout(91.133% 370/406)
> 			    schedule(95.135% 352/370)
> 			_raw_spin_unlock_irq(3.202% 13/406)
> 			_raw_spin_lock_irq(0.739% 3/406)
> 			srso_return_thunk(0.246% 1/406)
> 		    _raw_spin_unlock_irq(7.021% 33/470)
> 		    __call_rcu_common.constprop.0(3.830% 18/470)
> 		rcu_gp_is_expedited(3.036% 16/527)
> 		__cond_resched(0.569% 3/527)
> 		srso_return_thunk(0.190% 1/527)
> 
> They all call wait_for_completion, which may sleep on something on 
> purpose, I guess...

That's OK because ip_set_destroy() calls rcu_barrier() which is needed to 
handle flush in list type of sets.

However, rcu_barrier() with call_rcu() together makes multiple destroys 
one after another slow. But rcu_barrier() is needed for list type of sets 
only and that can be handled separately. So could you test the patch 
below? According to my tests it is even a little bit faster than the 
original code before synchronize_rcu() was added to swap.
  
diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
index e8c350a3ade1..912f750d0bea 100644
--- a/include/linux/netfilter/ipset/ip_set.h
+++ b/include/linux/netfilter/ipset/ip_set.h
@@ -242,6 +242,8 @@ extern void ip_set_type_unregister(struct ip_set_type *set_type);
 
 /* A generic IP set */
 struct ip_set {
+	/* For call_cru in destroy */
+	struct rcu_head rcu;
 	/* The name of the set */
 	char name[IPSET_MAXNAMELEN];
 	/* Lock protecting the set data */
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 4c133e06be1d..3bf9bb345809 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1182,6 +1182,14 @@ ip_set_destroy_set(struct ip_set *set)
 	kfree(set);
 }
 
+static void
+ip_set_destroy_set_rcu(struct rcu_head *head)
+{
+	struct ip_set *set = container_of(head, struct ip_set, rcu);
+
+	ip_set_destroy_set(set);
+}
+
 static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 			  const struct nlattr * const attr[])
 {
@@ -1193,8 +1201,6 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 	if (unlikely(protocol_min_failed(attr)))
 		return -IPSET_ERR_PROTOCOL;
 
-	/* Must wait for flush to be really finished in list:set */
-	rcu_barrier();
 
 	/* Commands are serialized and references are
 	 * protected by the ip_set_ref_lock.
@@ -1206,8 +1212,10 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 	 * counter, so if it's already zero, we can proceed
 	 * without holding the lock.
 	 */
-	read_lock_bh(&ip_set_ref_lock);
 	if (!attr[IPSET_ATTR_SETNAME]) {
+		/* Must wait for flush to be really finished in list:set */
+		rcu_barrier();
+		read_lock_bh(&ip_set_ref_lock);
 		for (i = 0; i < inst->ip_set_max; i++) {
 			s = ip_set(inst, i);
 			if (s && (s->ref || s->ref_netlink)) {
@@ -1228,6 +1236,9 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 		inst->is_destroyed = false;
 	} else {
 		u32 flags = flag_exist(info->nlh);
+		u16 features = 0;
+
+		read_lock_bh(&ip_set_ref_lock);
 		s = find_set_and_id(inst, nla_data(attr[IPSET_ATTR_SETNAME]),
 				    &i);
 		if (!s) {
@@ -1238,10 +1249,14 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
 			ret = -IPSET_ERR_BUSY;
 			goto out;
 		}
+		features = s->type->features;
 		ip_set(inst, i) = NULL;
 		read_unlock_bh(&ip_set_ref_lock);
-
-		ip_set_destroy_set(s);
+		if (features & IPSET_TYPE_NAME) {
+			/* Must wait for flush to be really finished  */
+			rcu_barrier();
+		}
+		call_rcu(&s->rcu, ip_set_destroy_set_rcu);
 	}
 	return 0;
 out:
@@ -1394,9 +1409,6 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info,
 	ip_set(inst, to_id) = from;
 	write_unlock_bh(&ip_set_ref_lock);
 
-	/* Make sure all readers of the old set pointers are completed. */
-	synchronize_rcu();
-
 	return 0;
 }
 
@@ -2357,6 +2369,9 @@ ip_set_net_exit(struct net *net)
 
 	inst->is_deleted = true; /* flag for ip_set_nfnl_put */
 
+	/* Wait for call_rcu() in destroy */
+	rcu_barrier();
+
 	nfnl_lock(NFNL_SUBSYS_IPSET);
 	for (i = 0; i < inst->ip_set_max; i++) {
 		set = ip_set(inst, i);


Best regards,
Jozsef
-- 
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re:Performance regression in ip_set_swap on 6.7.0

[David Wang]

At 2024-01-14 02:24:07, "Jozsef Kadlecsik" <kadlec@blackhole.kfki.hu> wrote:
>On Thu, 11 Jan 2024, David Wang wrote:
>
>> I tested the patch with code stressing swap->destroy->create->add 10000 
>> times, the performance regression still happens, and now it is 
>> ip_set_destroy. (I pasted the test code at the end of this mail)

>> 
>> They all call wait_for_completion, which may sleep on something on 
>> purpose, I guess...
>
>That's OK because ip_set_destroy() calls rcu_barrier() which is needed to 
>handle flush in list type of sets.
>
>However, rcu_barrier() with call_rcu() together makes multiple destroys 
>one after another slow. But rcu_barrier() is needed for list type of sets 
>only and that can be handled separately. So could you test the patch 
>below? According to my tests it is even a little bit faster than the 
>original code before synchronize_rcu() was added to swap.

Confirmed~! This patch does fix the performance regression in my case.

Hope it can fix ale.crismani@automattic.com's original issue.



Thanks~
David

Re: Performance regression in ip_set_swap on 6.7.0

[Ale Crismani]

> Il giorno 14 gen 2024, alle ore 06:30, David Wang <00107082@163.com> ha scritto:
> 
> 
> At 2024-01-14 02:24:07, "Jozsef Kadlecsik" <kadlec@blackhole.kfki.hu> wrote:
>> On Thu, 11 Jan 2024, David Wang wrote:
>> 
>>> I tested the patch with code stressing swap->destroy->create->add 10000 
>>> times, the performance regression still happens, and now it is 
>>> ip_set_destroy. (I pasted the test code at the end of this mail)
> 
>>> 
>>> They all call wait_for_completion, which may sleep on something on 
>>> purpose, I guess...
>> 
>> That's OK because ip_set_destroy() calls rcu_barrier() which is needed to 
>> handle flush in list type of sets.
>> 
>> However, rcu_barrier() with call_rcu() together makes multiple destroys 
>> one after another slow. But rcu_barrier() is needed for list type of sets 
>> only and that can be handled separately. So could you test the patch 
>> below? According to my tests it is even a little bit faster than the 
>> original code before synchronize_rcu() was added to swap.
> 
> Confirmed~! This patch does fix the performance regression in my case.
> 
> Hope it can fix ale.crismani@automattic.com's original issue.
> 
> 
> 
> Thanks~
> David


Thanks for all the help on this, I'll try the patch tomorrow hopefully and will report back!

best wishes,
Ale

Re: Performance regression in ip_set_swap on 6.7.0

[Ale Crismani]

> Il giorno 14 gen 2024, alle ore 21:38, Ale Crismani <ale.crismani@automattic.com> ha scritto:
> 
> 
> 
>> Il giorno 14 gen 2024, alle ore 06:30, David Wang <00107082@163.com> ha scritto:
>> 
>> 
>> At 2024-01-14 02:24:07, "Jozsef Kadlecsik" <kadlec@blackhole.kfki.hu> wrote:
>>> On Thu, 11 Jan 2024, David Wang wrote:
>>> 
>>>> I tested the patch with code stressing swap->destroy->create->add 10000 
>>>> times, the performance regression still happens, and now it is 
>>>> ip_set_destroy. (I pasted the test code at the end of this mail)
>> 
>>>> 
>>>> They all call wait_for_completion, which may sleep on something on 
>>>> purpose, I guess...
>>> 
>>> That's OK because ip_set_destroy() calls rcu_barrier() which is needed to 
>>> handle flush in list type of sets.
>>> 
>>> However, rcu_barrier() with call_rcu() together makes multiple destroys 
>>> one after another slow. But rcu_barrier() is needed for list type of sets 
>>> only and that can be handled separately. So could you test the patch 
>>> below? According to my tests it is even a little bit faster than the 
>>> original code before synchronize_rcu() was added to swap.
>> 
>> Confirmed~! This patch does fix the performance regression in my case.
>> 
>> Hope it can fix ale.crismani@automattic.com's original issue.
>> 
>> 
>> 
>> Thanks~
>> David
> 
> 
> Thanks for all the help on this, I'll try the patch tomorrow hopefully and will report back!
> 
> best wishes,
> Ale


I applied the patch on 6.1.y on top of 875ee3a and I can confirm it fixes the performance issues in our case too.

Thanks once more for having looked at this!
Ale

Re: Performance regression in ip_set_swap on 6.7.0

[Jozsef Kadlecsik]

On Mon, 15 Jan 2024, Ale Crismani wrote:

> > Il giorno 14 gen 2024, alle ore 21:38, Ale Crismani <ale.crismani@automattic.com> ha scritto:
> > 
> >> Il giorno 14 gen 2024, alle ore 06:30, David Wang <00107082@163.com> ha scritto:
> >> 
> >> 
> >> At 2024-01-14 02:24:07, "Jozsef Kadlecsik" <kadlec@blackhole.kfki.hu> wrote:
> >>> On Thu, 11 Jan 2024, David Wang wrote:
> >>> 
> >>>> I tested the patch with code stressing swap->destroy->create->add 10000 
> >>>> times, the performance regression still happens, and now it is 
> >>>> ip_set_destroy. (I pasted the test code at the end of this mail)
> >> 
> >>>> 
> >>>> They all call wait_for_completion, which may sleep on something on 
> >>>> purpose, I guess...
> >>> 
> >>> That's OK because ip_set_destroy() calls rcu_barrier() which is needed to 
> >>> handle flush in list type of sets.
> >>> 
> >>> However, rcu_barrier() with call_rcu() together makes multiple destroys 
> >>> one after another slow. But rcu_barrier() is needed for list type of sets 
> >>> only and that can be handled separately. So could you test the patch 
> >>> below? According to my tests it is even a little bit faster than the 
> >>> original code before synchronize_rcu() was added to swap.
> >> 
> >> Confirmed~! This patch does fix the performance regression in my case.
> >> 
> >> Hope it can fix ale.crismani@automattic.com's original issue.
> > 
> > Thanks for all the help on this, I'll try the patch tomorrow hopefully 
> > and will report back!
> 
> I applied the patch on 6.1.y on top of 875ee3a and I can confirm it 
> fixes the performance issues in our case too.

Thanks for the testing, to both of you. I'm going to release the patch 
for kernel inclusion.

Best regards,
Jozsef
-- 
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary