diff mbox series

nscd: bump GC cycle during cache pruning (bug 26130)

Message ID mvm1rjb9ezq.fsf@suse.de
State Committed
Commit 5e74e6f85842892bc25da8e8c70d8dadd485941a
Headers show
Series nscd: bump GC cycle during cache pruning (bug 26130) | expand

Commit Message

Andreas Schwab Sept. 9, 2020, 2:55 p.m. UTC
While nscd prunes a cache it becomes inconsistent temporarily, which is
visible to clients if that cache is shared.  Bump the GC cycle counter so
that the clients notice the modification window.

Uniformly use atomic_fetch_add to modify the GC cycle counter.
---
 nscd/cache.c | 9 +++++++++
 nscd/mem.c   | 4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

Comments

Adhemerval Zanella Sept. 9, 2020, 6:43 p.m. UTC | #1
On 09/09/2020 11:55, Andreas Schwab wrote:
> While nscd prunes a cache it becomes inconsistent temporarily, which is
> visible to clients if that cache is shared.  Bump the GC cycle counter so
> that the clients notice the modification window.
> 
> Uniformly use atomic_fetch_add to modify the GC cycle counter.
> ---
>  nscd/cache.c | 9 +++++++++
>  nscd/mem.c   | 4 ++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/nscd/cache.c b/nscd/cache.c
> index 94163d9ce3..c0b9b4da3d 100644
> --- a/nscd/cache.c
> +++ b/nscd/cache.c
> @@ -453,6 +453,11 @@ prune_cache (struct database_dyn *table, time_t now, int fd)
>  	  pthread_rwlock_wrlock (&table->lock);
>  	}
>  
> +      /* Now we start modifying the data.  Make sure all readers of the
> +	 data are aware of this and temporarily don't use the data.  */
> +      atomic_fetch_add_relaxed (&table->head->gc_cycle, 1);
> +      assert ((table->head->gc_cycle & 1) == 1);
> +


Is a relaxed atomic suffice here? Other accesses are not done with atomic
at all, so I am not sure how well defined are the 'gc_cycle' usage.

>        while (first <= last)
>  	{
>  	  if (mark[first])
> @@ -493,6 +498,10 @@ prune_cache (struct database_dyn *table, time_t now, int fd)
>  	  ++first;
>  	}
>  
> +      /* Now we are done modifying the data.  */
> +      atomic_fetch_add_relaxed (&table->head->gc_cycle, 1);
> +      assert ((table->head->gc_cycle & 1) == 0);
> +
>        /* It's all done.  */
>        pthread_rwlock_unlock (&table->lock);
>  
> diff --git a/nscd/mem.c b/nscd/mem.c
> index 3d10bb9b46..de5bd12db5 100644
> --- a/nscd/mem.c
> +++ b/nscd/mem.c
> @@ -264,7 +264,7 @@ gc (struct database_dyn *db)
>  
>    /* Now we start modifying the data.  Make sure all readers of the
>       data are aware of this and temporarily don't use the data.  */
> -  ++db->head->gc_cycle;
> +  atomic_fetch_add_relaxed (&db->head->gc_cycle, 1);
>    assert ((db->head->gc_cycle & 1) == 1);
>  
>  
> @@ -490,7 +490,7 @@ gc (struct database_dyn *db)
>  
>  
>    /* Now we are done modifying the data.  */
> -  ++db->head->gc_cycle;
> +  atomic_fetch_add_relaxed (&db->head->gc_cycle, 1);
>    assert ((db->head->gc_cycle & 1) == 0);
>  
>    /* We are done.  */
>
Andreas Schwab Sept. 10, 2020, 8:46 a.m. UTC | #2
On Sep 09 2020, Adhemerval Zanella wrote:

> Is a relaxed atomic suffice here?

I don't really know.

> Other accesses are not done with atomic at all, so I am not sure how
> well defined are the 'gc_cycle' usage.

It won't be worse the the current state, will it?

Andreas.
Florian Weimer Sept. 10, 2020, 9:58 a.m. UTC | #3
* Andreas Schwab:

> On Sep 09 2020, Adhemerval Zanella wrote:
>
>> Is a relaxed atomic suffice here?
>
> I don't really know.
>
>> Other accesses are not done with atomic at all, so I am not sure how
>> well defined are the 'gc_cycle' usage.
>
> It won't be worse the the current state, will it?

I think that's true, but in the past, Carlos and Torvald both objected
very strongly to partial improvements in code suffering from concurrency
issues.  I'd say give them a few days to comment, and commit your patch
some time next week.

Thanks,
Florian
diff mbox series

Patch

diff --git a/nscd/cache.c b/nscd/cache.c
index 94163d9ce3..c0b9b4da3d 100644
--- a/nscd/cache.c
+++ b/nscd/cache.c
@@ -453,6 +453,11 @@  prune_cache (struct database_dyn *table, time_t now, int fd)
 	  pthread_rwlock_wrlock (&table->lock);
 	}
 
+      /* Now we start modifying the data.  Make sure all readers of the
+	 data are aware of this and temporarily don't use the data.  */
+      atomic_fetch_add_relaxed (&table->head->gc_cycle, 1);
+      assert ((table->head->gc_cycle & 1) == 1);
+
       while (first <= last)
 	{
 	  if (mark[first])
@@ -493,6 +498,10 @@  prune_cache (struct database_dyn *table, time_t now, int fd)
 	  ++first;
 	}
 
+      /* Now we are done modifying the data.  */
+      atomic_fetch_add_relaxed (&table->head->gc_cycle, 1);
+      assert ((table->head->gc_cycle & 1) == 0);
+
       /* It's all done.  */
       pthread_rwlock_unlock (&table->lock);
 
diff --git a/nscd/mem.c b/nscd/mem.c
index 3d10bb9b46..de5bd12db5 100644
--- a/nscd/mem.c
+++ b/nscd/mem.c
@@ -264,7 +264,7 @@  gc (struct database_dyn *db)
 
   /* Now we start modifying the data.  Make sure all readers of the
      data are aware of this and temporarily don't use the data.  */
-  ++db->head->gc_cycle;
+  atomic_fetch_add_relaxed (&db->head->gc_cycle, 1);
   assert ((db->head->gc_cycle & 1) == 1);
 
 
@@ -490,7 +490,7 @@  gc (struct database_dyn *db)
 
 
   /* Now we are done modifying the data.  */
-  ++db->head->gc_cycle;
+  atomic_fetch_add_relaxed (&db->head->gc_cycle, 1);
   assert ((db->head->gc_cycle & 1) == 0);
 
   /* We are done.  */