diff mbox series

[10/13] ipvs: Rename kfree_rcu() to kfree_rcu_mightsleep()

Message ID 20230201150954.409693-1-urezki@gmail.com (mailing list archive)
State New, archived
Headers show
Series Rename k[v]free_rcu() single argument to k[v]free_rcu_mightsleep() | expand

Commit Message

Uladzislau Rezki Feb. 1, 2023, 3:09 p.m. UTC
The kfree_rcu()'s single argument name is deprecated therefore
rename it to kfree_rcu_mightsleep() variant. The goal is explicitly
underline that it is for sleepable contexts.

Cc: Julian Anastasov <ja@ssi.bg>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jiri Wiesner <jwiesner@suse.de>
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 net/netfilter/ipvs/ip_vs_est.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Pablo Neira Ayuso Feb. 1, 2023, 3:53 p.m. UTC | #1
Hi,

On Wed, Feb 01, 2023 at 04:09:51PM +0100, Uladzislau Rezki (Sony) wrote:
> The kfree_rcu()'s single argument name is deprecated therefore
> rename it to kfree_rcu_mightsleep() variant. The goal is explicitly
> underline that it is for sleepable contexts.
> 
> Cc: Julian Anastasov <ja@ssi.bg>
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: Jiri Wiesner <jwiesner@suse.de>
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  net/netfilter/ipvs/ip_vs_est.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> index ce2a1549b304..a39baf6d1367 100644
> --- a/net/netfilter/ipvs/ip_vs_est.c
> +++ b/net/netfilter/ipvs/ip_vs_est.c
> @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
>  	__set_bit(row, kd->avail);
>  	if (!kd->tick_len[row]) {
>  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> -		kfree_rcu(td);

I also found this kfree_rcu() without rcu_head call a few weeks ago.

@Wiesner, @Julian: Any chance this can be turned into kfree_rcu(td, rcu_head); ?
Julian Anastasov Feb. 1, 2023, 4:12 p.m. UTC | #2
Hello,

On Wed, 1 Feb 2023, Pablo Neira Ayuso wrote:

> Hi,
> 
> On Wed, Feb 01, 2023 at 04:09:51PM +0100, Uladzislau Rezki (Sony) wrote:
> > The kfree_rcu()'s single argument name is deprecated therefore
> > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly
> > underline that it is for sleepable contexts.
> > 
> > Cc: Julian Anastasov <ja@ssi.bg>
> > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > Cc: Jiri Wiesner <jwiesner@suse.de>
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > ---
> >  net/netfilter/ipvs/ip_vs_est.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > index ce2a1549b304..a39baf6d1367 100644
> > --- a/net/netfilter/ipvs/ip_vs_est.c
> > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> >  	__set_bit(row, kd->avail);
> >  	if (!kd->tick_len[row]) {
> >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > -		kfree_rcu(td);
> 
> I also found this kfree_rcu() without rcu_head call a few weeks ago.
> 
> @Wiesner, @Julian: Any chance this can be turned into kfree_rcu(td, rcu_head); ?

	Yes, as simple as this:

diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index c6c61100d244..6d71a5ff52df 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -461,6 +461,7 @@ void ip_vs_stats_free(struct ip_vs_stats *stats);
 
 /* Multiple chains processed in same tick */
 struct ip_vs_est_tick_data {
+	struct rcu_head		rcu_head;
 	struct hlist_head	chains[IPVS_EST_TICK_CHAINS];
 	DECLARE_BITMAP(present, IPVS_EST_TICK_CHAINS);
 	DECLARE_BITMAP(full, IPVS_EST_TICK_CHAINS);
diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
index df56073bb282..25c7118d9348 100644
--- a/net/netfilter/ipvs/ip_vs_est.c
+++ b/net/netfilter/ipvs/ip_vs_est.c
@@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
 	__set_bit(row, kd->avail);
 	if (!kd->tick_len[row]) {
 		RCU_INIT_POINTER(kd->ticks[row], NULL);
-		kfree_rcu(td);
+		kfree_rcu(td, rcu_head);
 	}
 	kd->est_count--;
 	if (kd->est_count) {

	I was about to reply to Uladzislau Rezki but his patchset
looks more like a renaming, so I'm not sure how we are about
to integrate this change, as separate patch or as part of his
patchset. I don't have preference, just let me know how to
handle it.

Regards

--
Julian Anastasov <ja@ssi.bg>
Uladzislau Rezki Feb. 1, 2023, 4:52 p.m. UTC | #3
On Wed, Feb 01, 2023 at 06:12:04PM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Wed, 1 Feb 2023, Pablo Neira Ayuso wrote:
> 
> > Hi,
> > 
> > On Wed, Feb 01, 2023 at 04:09:51PM +0100, Uladzislau Rezki (Sony) wrote:
> > > The kfree_rcu()'s single argument name is deprecated therefore
> > > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly
> > > underline that it is for sleepable contexts.
> > > 
> > > Cc: Julian Anastasov <ja@ssi.bg>
> > > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > > Cc: Jiri Wiesner <jwiesner@suse.de>
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > ---
> > >  net/netfilter/ipvs/ip_vs_est.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > > index ce2a1549b304..a39baf6d1367 100644
> > > --- a/net/netfilter/ipvs/ip_vs_est.c
> > > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> > >  	__set_bit(row, kd->avail);
> > >  	if (!kd->tick_len[row]) {
> > >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > > -		kfree_rcu(td);
> > 
> > I also found this kfree_rcu() without rcu_head call a few weeks ago.
> > 
> > @Wiesner, @Julian: Any chance this can be turned into kfree_rcu(td, rcu_head); ?
> 
> 	Yes, as simple as this:
> 
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index c6c61100d244..6d71a5ff52df 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -461,6 +461,7 @@ void ip_vs_stats_free(struct ip_vs_stats *stats);
>  
>  /* Multiple chains processed in same tick */
>  struct ip_vs_est_tick_data {
> +	struct rcu_head		rcu_head;
>  	struct hlist_head	chains[IPVS_EST_TICK_CHAINS];
>  	DECLARE_BITMAP(present, IPVS_EST_TICK_CHAINS);
>  	DECLARE_BITMAP(full, IPVS_EST_TICK_CHAINS);
> diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> index df56073bb282..25c7118d9348 100644
> --- a/net/netfilter/ipvs/ip_vs_est.c
> +++ b/net/netfilter/ipvs/ip_vs_est.c
> @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
>  	__set_bit(row, kd->avail);
>  	if (!kd->tick_len[row]) {
>  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> -		kfree_rcu(td);
> +		kfree_rcu(td, rcu_head);
>  	}
>  	kd->est_count--;
>  	if (kd->est_count) {
> 
> 	I was about to reply to Uladzislau Rezki but his patchset
> looks more like a renaming, so I'm not sure how we are about
> to integrate this change, as separate patch or as part of his
> patchset. I don't have preference, just let me know how to
> handle it.
> 
If you give on this patch an Ack i will resend it once i collect
all other ACKs. So it will be integrated via RCU-tree.

So this is just renaming.

If the ip_vs_stop_estimator() can be invoked from an atomic context
then it is a bug and you need to integrate rcu_head in your structure.

--
Uladzislau Rezki
Pablo Neira Ayuso Feb. 1, 2023, 5:10 p.m. UTC | #4
On Wed, Feb 01, 2023 at 06:12:04PM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Wed, 1 Feb 2023, Pablo Neira Ayuso wrote:
> 
> > Hi,
> > 
> > On Wed, Feb 01, 2023 at 04:09:51PM +0100, Uladzislau Rezki (Sony) wrote:
> > > The kfree_rcu()'s single argument name is deprecated therefore
> > > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly
> > > underline that it is for sleepable contexts.
> > > 
> > > Cc: Julian Anastasov <ja@ssi.bg>
> > > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > > Cc: Jiri Wiesner <jwiesner@suse.de>
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > ---
> > >  net/netfilter/ipvs/ip_vs_est.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > > index ce2a1549b304..a39baf6d1367 100644
> > > --- a/net/netfilter/ipvs/ip_vs_est.c
> > > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> > >  	__set_bit(row, kd->avail);
> > >  	if (!kd->tick_len[row]) {
> > >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > > -		kfree_rcu(td);
> > 
> > I also found this kfree_rcu() without rcu_head call a few weeks ago.
> > 
> > @Wiesner, @Julian: Any chance this can be turned into kfree_rcu(td, rcu_head); ?
> 
> 	Yes, as simple as this:
> 
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index c6c61100d244..6d71a5ff52df 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -461,6 +461,7 @@ void ip_vs_stats_free(struct ip_vs_stats *stats);
>  
>  /* Multiple chains processed in same tick */
>  struct ip_vs_est_tick_data {
> +	struct rcu_head		rcu_head;
>  	struct hlist_head	chains[IPVS_EST_TICK_CHAINS];
>  	DECLARE_BITMAP(present, IPVS_EST_TICK_CHAINS);
>  	DECLARE_BITMAP(full, IPVS_EST_TICK_CHAINS);
> diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> index df56073bb282..25c7118d9348 100644
> --- a/net/netfilter/ipvs/ip_vs_est.c
> +++ b/net/netfilter/ipvs/ip_vs_est.c
> @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
>  	__set_bit(row, kd->avail);
>  	if (!kd->tick_len[row]) {
>  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> -		kfree_rcu(td);
> +		kfree_rcu(td, rcu_head);
>  	}
>  	kd->est_count--;
>  	if (kd->est_count) {
> 
> 	I was about to reply to Uladzislau Rezki but his patchset
> looks more like a renaming, so I'm not sure how we are about
> to integrate this change, as separate patch or as part of his
> patchset. I don't have preference, just let me know how to
> handle it.

@Uladzislau Rezki: Are you fine with dropping this patch from your
series and Julian will send us a patch for inclusion into net-next to
use the kfree_rcu(x, rcu_head) variant?
Uladzislau Rezki Feb. 1, 2023, 5:19 p.m. UTC | #5
On Wed, Feb 01, 2023 at 06:10:18PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Feb 01, 2023 at 06:12:04PM +0200, Julian Anastasov wrote:
> > 
> > 	Hello,
> > 
> > On Wed, 1 Feb 2023, Pablo Neira Ayuso wrote:
> > 
> > > Hi,
> > > 
> > > On Wed, Feb 01, 2023 at 04:09:51PM +0100, Uladzislau Rezki (Sony) wrote:
> > > > The kfree_rcu()'s single argument name is deprecated therefore
> > > > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly
> > > > underline that it is for sleepable contexts.
> > > > 
> > > > Cc: Julian Anastasov <ja@ssi.bg>
> > > > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > Cc: Jiri Wiesner <jwiesner@suse.de>
> > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > ---
> > > >  net/netfilter/ipvs/ip_vs_est.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > > > index ce2a1549b304..a39baf6d1367 100644
> > > > --- a/net/netfilter/ipvs/ip_vs_est.c
> > > > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > > > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> > > >  	__set_bit(row, kd->avail);
> > > >  	if (!kd->tick_len[row]) {
> > > >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > > > -		kfree_rcu(td);
> > > 
> > > I also found this kfree_rcu() without rcu_head call a few weeks ago.
> > > 
> > > @Wiesner, @Julian: Any chance this can be turned into kfree_rcu(td, rcu_head); ?
> > 
> > 	Yes, as simple as this:
> > 
> > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> > index c6c61100d244..6d71a5ff52df 100644
> > --- a/include/net/ip_vs.h
> > +++ b/include/net/ip_vs.h
> > @@ -461,6 +461,7 @@ void ip_vs_stats_free(struct ip_vs_stats *stats);
> >  
> >  /* Multiple chains processed in same tick */
> >  struct ip_vs_est_tick_data {
> > +	struct rcu_head		rcu_head;
> >  	struct hlist_head	chains[IPVS_EST_TICK_CHAINS];
> >  	DECLARE_BITMAP(present, IPVS_EST_TICK_CHAINS);
> >  	DECLARE_BITMAP(full, IPVS_EST_TICK_CHAINS);
> > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > index df56073bb282..25c7118d9348 100644
> > --- a/net/netfilter/ipvs/ip_vs_est.c
> > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> >  	__set_bit(row, kd->avail);
> >  	if (!kd->tick_len[row]) {
> >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > -		kfree_rcu(td);
> > +		kfree_rcu(td, rcu_head);
> >  	}
> >  	kd->est_count--;
> >  	if (kd->est_count) {
> > 
> > 	I was about to reply to Uladzislau Rezki but his patchset
> > looks more like a renaming, so I'm not sure how we are about
> > to integrate this change, as separate patch or as part of his
> > patchset. I don't have preference, just let me know how to
> > handle it.
> 
> @Uladzislau Rezki: Are you fine with dropping this patch from your
> series and Julian will send us a patch for inclusion into net-next to
> use the kfree_rcu(x, rcu_head) variant?
Absolutely. So i will drop it from my series.

Thanks!

--
Uladzislau Rezki
Joel Fernandes March 9, 2023, 12:11 a.m. UTC | #6
Hello,

On Wed, Feb 01, 2023 at 06:19:49PM +0100, Uladzislau Rezki wrote:
> On Wed, Feb 01, 2023 at 06:10:18PM +0100, Pablo Neira Ayuso wrote:
> > On Wed, Feb 01, 2023 at 06:12:04PM +0200, Julian Anastasov wrote:
> > > 
> > > 	Hello,
> > > 
> > > On Wed, 1 Feb 2023, Pablo Neira Ayuso wrote:
> > > 
> > > > Hi,
> > > > 
> > > > On Wed, Feb 01, 2023 at 04:09:51PM +0100, Uladzislau Rezki (Sony) wrote:
> > > > > The kfree_rcu()'s single argument name is deprecated therefore
> > > > > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly
> > > > > underline that it is for sleepable contexts.
> > > > > 
> > > > > Cc: Julian Anastasov <ja@ssi.bg>
> > > > > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > > Cc: Jiri Wiesner <jwiesner@suse.de>
> > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > > ---
> > > > >  net/netfilter/ipvs/ip_vs_est.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > > > > index ce2a1549b304..a39baf6d1367 100644
> > > > > --- a/net/netfilter/ipvs/ip_vs_est.c
> > > > > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > > > > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> > > > >  	__set_bit(row, kd->avail);
> > > > >  	if (!kd->tick_len[row]) {
> > > > >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > > > > -		kfree_rcu(td);
> > > > 
> > > > I also found this kfree_rcu() without rcu_head call a few weeks ago.
> > > > 
> > > > @Wiesner, @Julian: Any chance this can be turned into kfree_rcu(td, rcu_head); ?
> > > 
> > > 	Yes, as simple as this:
> > > 
> > > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> > > index c6c61100d244..6d71a5ff52df 100644
> > > --- a/include/net/ip_vs.h
> > > +++ b/include/net/ip_vs.h
> > > @@ -461,6 +461,7 @@ void ip_vs_stats_free(struct ip_vs_stats *stats);
> > >  
> > >  /* Multiple chains processed in same tick */
> > >  struct ip_vs_est_tick_data {
> > > +	struct rcu_head		rcu_head;
> > >  	struct hlist_head	chains[IPVS_EST_TICK_CHAINS];
> > >  	DECLARE_BITMAP(present, IPVS_EST_TICK_CHAINS);
> > >  	DECLARE_BITMAP(full, IPVS_EST_TICK_CHAINS);
> > > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > > index df56073bb282..25c7118d9348 100644
> > > --- a/net/netfilter/ipvs/ip_vs_est.c
> > > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> > >  	__set_bit(row, kd->avail);
> > >  	if (!kd->tick_len[row]) {
> > >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > > -		kfree_rcu(td);
> > > +		kfree_rcu(td, rcu_head);
> > >  	}
> > >  	kd->est_count--;
> > >  	if (kd->est_count) {
> > > 
> > > 	I was about to reply to Uladzislau Rezki but his patchset
> > > looks more like a renaming, so I'm not sure how we are about
> > > to integrate this change, as separate patch or as part of his
> > > patchset. I don't have preference, just let me know how to
> > > handle it.
> > 
> > @Uladzislau Rezki: Are you fine with dropping this patch from your
> > series and Julian will send us a patch for inclusion into net-next to
> > use the kfree_rcu(x, rcu_head) variant?
> Absolutely. So i will drop it from my series.
> 

Since this patch was dropped, it is the only case blocking the proper
integration of this series into linux-next. We want to drop the old API and
currently we are not able to, thus this revert [1] has to be unfortunately
carried in linux-next.

For that reason, there are 2 options:

1. Can we get the new rcu_head approach for ipvs posted and reviewed with
suitable Acks?

2. Can we carry Vlad's patch to use kfree_rcu_mightsleep() in ipvs and drop
it later if/when #1 is completed?

Option 2 has the unfortunate effect that it will conflict with your new
approach of using rcu_head so I'd rather you fix it that way and get it
Acked. And once acked, we can also take it via the RCU tree if the net
maintainers are Ok with that.

Please advise.

thanks,

 - Joel

[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=3898e7642316732e23716ca902f9d122736f9805
Pablo Neira Ayuso March 9, 2023, 9:19 a.m. UTC | #7
On Thu, Mar 09, 2023 at 12:11:01AM +0000, Joel Fernandes wrote:
> Hello,
> 
> On Wed, Feb 01, 2023 at 06:19:49PM +0100, Uladzislau Rezki wrote:
> > On Wed, Feb 01, 2023 at 06:10:18PM +0100, Pablo Neira Ayuso wrote:
> > > On Wed, Feb 01, 2023 at 06:12:04PM +0200, Julian Anastasov wrote:
> > > > 
> > > > 	Hello,
> > > > 
> > > > On Wed, 1 Feb 2023, Pablo Neira Ayuso wrote:
> > > > 
> > > > > Hi,
> > > > > 
> > > > > On Wed, Feb 01, 2023 at 04:09:51PM +0100, Uladzislau Rezki (Sony) wrote:
> > > > > > The kfree_rcu()'s single argument name is deprecated therefore
> > > > > > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly
> > > > > > underline that it is for sleepable contexts.
> > > > > > 
> > > > > > Cc: Julian Anastasov <ja@ssi.bg>
> > > > > > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > > > Cc: Jiri Wiesner <jwiesner@suse.de>
> > > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > > > ---
> > > > > >  net/netfilter/ipvs/ip_vs_est.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > > > > > index ce2a1549b304..a39baf6d1367 100644
> > > > > > --- a/net/netfilter/ipvs/ip_vs_est.c
> > > > > > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > > > > > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> > > > > >  	__set_bit(row, kd->avail);
> > > > > >  	if (!kd->tick_len[row]) {
> > > > > >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > > > > > -		kfree_rcu(td);
> > > > > 
> > > > > I also found this kfree_rcu() without rcu_head call a few weeks ago.
> > > > > 
> > > > > @Wiesner, @Julian: Any chance this can be turned into kfree_rcu(td, rcu_head); ?
> > > > 
> > > > 	Yes, as simple as this:
> > > > 
> > > > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> > > > index c6c61100d244..6d71a5ff52df 100644
> > > > --- a/include/net/ip_vs.h
> > > > +++ b/include/net/ip_vs.h
> > > > @@ -461,6 +461,7 @@ void ip_vs_stats_free(struct ip_vs_stats *stats);
> > > >  
> > > >  /* Multiple chains processed in same tick */
> > > >  struct ip_vs_est_tick_data {
> > > > +	struct rcu_head		rcu_head;
> > > >  	struct hlist_head	chains[IPVS_EST_TICK_CHAINS];
> > > >  	DECLARE_BITMAP(present, IPVS_EST_TICK_CHAINS);
> > > >  	DECLARE_BITMAP(full, IPVS_EST_TICK_CHAINS);
> > > > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > > > index df56073bb282..25c7118d9348 100644
> > > > --- a/net/netfilter/ipvs/ip_vs_est.c
> > > > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > > > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> > > >  	__set_bit(row, kd->avail);
> > > >  	if (!kd->tick_len[row]) {
> > > >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > > > -		kfree_rcu(td);
> > > > +		kfree_rcu(td, rcu_head);
> > > >  	}
> > > >  	kd->est_count--;
> > > >  	if (kd->est_count) {
> > > > 
> > > > 	I was about to reply to Uladzislau Rezki but his patchset
> > > > looks more like a renaming, so I'm not sure how we are about
> > > > to integrate this change, as separate patch or as part of his
> > > > patchset. I don't have preference, just let me know how to
> > > > handle it.
> > > 
> > > @Uladzislau Rezki: Are you fine with dropping this patch from your
> > > series and Julian will send us a patch for inclusion into net-next to
> > > use the kfree_rcu(x, rcu_head) variant?
> > Absolutely. So i will drop it from my series.
> 
> Since this patch was dropped, it is the only case blocking the proper
> integration of this series into linux-next. We want to drop the old API and
> currently we are not able to, thus this revert [1] has to be unfortunately
> carried in linux-next.
>
> For that reason, there are 2 options:
> 
> 1. Can we get the new rcu_head approach for ipvs posted and reviewed with
> suitable Acks?
> 
> 2. Can we carry Vlad's patch to use kfree_rcu_mightsleep() in ipvs and drop
> it later if/when #1 is completed?
> 
> Option 2 has the unfortunate effect that it will conflict with your new
> approach of using rcu_head so I'd rather you fix it that way and get it
> Acked. And once acked, we can also take it via the RCU tree if the net
> maintainers are Ok with that.
> 
> Please advise.

JFYI, this patch is already in linux.git

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e4d0fe71f59dc5137a2793ff7560730d80d1e1f4
Joel Fernandes March 10, 2023, 1:08 a.m. UTC | #8
On Thu, Mar 09, 2023 at 10:19:43AM +0100, Pablo Neira Ayuso wrote:
> On Thu, Mar 09, 2023 at 12:11:01AM +0000, Joel Fernandes wrote:
> > Hello,
> > 
> > On Wed, Feb 01, 2023 at 06:19:49PM +0100, Uladzislau Rezki wrote:
> > > On Wed, Feb 01, 2023 at 06:10:18PM +0100, Pablo Neira Ayuso wrote:
> > > > On Wed, Feb 01, 2023 at 06:12:04PM +0200, Julian Anastasov wrote:
> > > > > 
> > > > > 	Hello,
> > > > > 
> > > > > On Wed, 1 Feb 2023, Pablo Neira Ayuso wrote:
> > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On Wed, Feb 01, 2023 at 04:09:51PM +0100, Uladzislau Rezki (Sony) wrote:
> > > > > > > The kfree_rcu()'s single argument name is deprecated therefore
> > > > > > > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly
> > > > > > > underline that it is for sleepable contexts.
> > > > > > > 
> > > > > > > Cc: Julian Anastasov <ja@ssi.bg>
> > > > > > > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > > > > Cc: Jiri Wiesner <jwiesner@suse.de>
> > > > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > > > > ---
> > > > > > >  net/netfilter/ipvs/ip_vs_est.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > > > > > > index ce2a1549b304..a39baf6d1367 100644
> > > > > > > --- a/net/netfilter/ipvs/ip_vs_est.c
> > > > > > > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > > > > > > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> > > > > > >  	__set_bit(row, kd->avail);
> > > > > > >  	if (!kd->tick_len[row]) {
> > > > > > >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > > > > > > -		kfree_rcu(td);
> > > > > > 
> > > > > > I also found this kfree_rcu() without rcu_head call a few weeks ago.
> > > > > > 
> > > > > > @Wiesner, @Julian: Any chance this can be turned into kfree_rcu(td, rcu_head); ?
> > > > > 
> > > > > 	Yes, as simple as this:
> > > > > 
> > > > > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> > > > > index c6c61100d244..6d71a5ff52df 100644
> > > > > --- a/include/net/ip_vs.h
> > > > > +++ b/include/net/ip_vs.h
> > > > > @@ -461,6 +461,7 @@ void ip_vs_stats_free(struct ip_vs_stats *stats);
> > > > >  
> > > > >  /* Multiple chains processed in same tick */
> > > > >  struct ip_vs_est_tick_data {
> > > > > +	struct rcu_head		rcu_head;
> > > > >  	struct hlist_head	chains[IPVS_EST_TICK_CHAINS];
> > > > >  	DECLARE_BITMAP(present, IPVS_EST_TICK_CHAINS);
> > > > >  	DECLARE_BITMAP(full, IPVS_EST_TICK_CHAINS);
> > > > > diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
> > > > > index df56073bb282..25c7118d9348 100644
> > > > > --- a/net/netfilter/ipvs/ip_vs_est.c
> > > > > +++ b/net/netfilter/ipvs/ip_vs_est.c
> > > > > @@ -549,7 +549,7 @@ void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
> > > > >  	__set_bit(row, kd->avail);
> > > > >  	if (!kd->tick_len[row]) {
> > > > >  		RCU_INIT_POINTER(kd->ticks[row], NULL);
> > > > > -		kfree_rcu(td);
> > > > > +		kfree_rcu(td, rcu_head);
> > > > >  	}
> > > > >  	kd->est_count--;
> > > > >  	if (kd->est_count) {
> > > > > 
> > > > > 	I was about to reply to Uladzislau Rezki but his patchset
> > > > > looks more like a renaming, so I'm not sure how we are about
> > > > > to integrate this change, as separate patch or as part of his
> > > > > patchset. I don't have preference, just let me know how to
> > > > > handle it.
> > > > 
> > > > @Uladzislau Rezki: Are you fine with dropping this patch from your
> > > > series and Julian will send us a patch for inclusion into net-next to
> > > > use the kfree_rcu(x, rcu_head) variant?
> > > Absolutely. So i will drop it from my series.
> > 
> > Since this patch was dropped, it is the only case blocking the proper
> > integration of this series into linux-next. We want to drop the old API and
> > currently we are not able to, thus this revert [1] has to be unfortunately
> > carried in linux-next.
> >
> > For that reason, there are 2 options:
> > 
> > 1. Can we get the new rcu_head approach for ipvs posted and reviewed with
> > suitable Acks?
> > 
> > 2. Can we carry Vlad's patch to use kfree_rcu_mightsleep() in ipvs and drop
> > it later if/when #1 is completed?
> > 
> > Option 2 has the unfortunate effect that it will conflict with your new
> > approach of using rcu_head so I'd rather you fix it that way and get it
> > Acked. And once acked, we can also take it via the RCU tree if the net
> > maintainers are Ok with that.
> > 
> > Please advise.
> 
> JFYI, this patch is already in linux.git
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e4d0fe71f59dc5137a2793ff7560730d80d1e1f4

Indeed it is in, thank you!

 - Joel
diff mbox series

Patch

diff --git a/net/netfilter/ipvs/ip_vs_est.c b/net/netfilter/ipvs/ip_vs_est.c
index ce2a1549b304..a39baf6d1367 100644
--- a/net/netfilter/ipvs/ip_vs_est.c
+++ b/net/netfilter/ipvs/ip_vs_est.c
@@ -549,7 +549,7 @@  void ip_vs_stop_estimator(struct netns_ipvs *ipvs, struct ip_vs_stats *stats)
 	__set_bit(row, kd->avail);
 	if (!kd->tick_len[row]) {
 		RCU_INIT_POINTER(kd->ticks[row], NULL);
-		kfree_rcu(td);
+		kfree_rcu_mightsleep(td);
 	}
 	kd->est_count--;
 	if (kd->est_count) {