* [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
@ 2020-06-17 14:10 Andreas Schwab
2020-06-29 8:27 ` Florian Weimer
0 siblings, 1 reply; 13+ messages in thread
From: Andreas Schwab @ 2020-06-17 14:10 UTC (permalink / raw)
To: libc-alpha
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.
---
nscd/cache.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/nscd/cache.c b/nscd/cache.c
index 94163d9ce3..2ceac94c23 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. */
+ ++table->head->gc_cycle;
+ 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. */
+ ++table->head->gc_cycle;
+ assert ((table->head->gc_cycle & 1) == 0);
+
/* It's all done. */
pthread_rwlock_unlock (&table->lock);
--
2.26.2
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
2020-06-17 14:10 [PATCH] nscd: bump GC cycle during cache pruning (bug 26130) Andreas Schwab
@ 2020-06-29 8:27 ` Florian Weimer
2020-06-29 10:35 ` Andreas Schwab
0 siblings, 1 reply; 13+ messages in thread
From: Florian Weimer @ 2020-06-29 8:27 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
* Andreas Schwab:
> 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.
> ---
> nscd/cache.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/nscd/cache.c b/nscd/cache.c
> index 94163d9ce3..2ceac94c23 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. */
> + ++table->head->gc_cycle;
> + 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. */
> + ++table->head->gc_cycle;
> + assert ((table->head->gc_cycle & 1) == 0);
> +
> /* It's all done. */
> pthread_rwlock_unlock (&table->lock);
I think this needs barriers after and before the increments.
Thanks,
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
2020-06-29 8:27 ` Florian Weimer
@ 2020-06-29 10:35 ` Andreas Schwab
2020-06-29 10:48 ` Florian Weimer
0 siblings, 1 reply; 13+ messages in thread
From: Andreas Schwab @ 2020-06-29 10:35 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
On Jun 29 2020, Florian Weimer wrote:
> I think this needs barriers after and before the increments.
Why doesn't gc need those barriers?
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
2020-06-29 10:35 ` Andreas Schwab
@ 2020-06-29 10:48 ` Florian Weimer
2020-06-29 14:09 ` Andreas Schwab
0 siblings, 1 reply; 13+ messages in thread
From: Florian Weimer @ 2020-06-29 10:48 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
* Andreas Schwab:
> On Jun 29 2020, Florian Weimer wrote:
>
>> I think this needs barriers after and before the increments.
>
> Why doesn't gc need those barriers?
I think it needs them as well. I thought we had them there. 8-(
Thanks,
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
2020-06-29 10:48 ` Florian Weimer
@ 2020-06-29 14:09 ` Andreas Schwab
2020-06-29 14:14 ` Carlos O'Donell
2020-06-29 14:16 ` Florian Weimer
0 siblings, 2 replies; 13+ messages in thread
From: Andreas Schwab @ 2020-06-29 14:09 UTC (permalink / raw)
To: Florian Weimer via Libc-alpha; +Cc: Florian Weimer
On Jun 29 2020, Florian Weimer via Libc-alpha wrote:
> * Andreas Schwab:
>
>> On Jun 29 2020, Florian Weimer wrote:
>>
>>> I think this needs barriers after and before the increments.
>>
>> Why doesn't gc need those barriers?
>
> I think it needs them as well. I thought we had them there. 8-(
Should they use atomic_increment?
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
2020-06-29 14:09 ` Andreas Schwab
@ 2020-06-29 14:14 ` Carlos O'Donell
2020-06-29 14:20 ` Andreas Schwab
2020-06-29 14:16 ` Florian Weimer
1 sibling, 1 reply; 13+ messages in thread
From: Carlos O'Donell @ 2020-06-29 14:14 UTC (permalink / raw)
To: Andreas Schwab, Florian Weimer via Libc-alpha; +Cc: Florian Weimer
On 6/29/20 10:09 AM, Andreas Schwab wrote:
> On Jun 29 2020, Florian Weimer via Libc-alpha wrote:
>
>> * Andreas Schwab:
>>
>>> On Jun 29 2020, Florian Weimer wrote:
>>>
>>>> I think this needs barriers after and before the increments.
>>>
>>> Why doesn't gc need those barriers?
>>
>> I think it needs them as well. I thought we had them there. 8-(
>
> Should they use atomic_increment?
There are many concurrency problems with the cache, and we do see
these issues with downstream customers. Torvald reviewed this at
one point and produced a patch that I've attached to bug 25888.
Our latest attempt to fix this is here:
https://sourceware.org/bugzilla/show_bug.cgi?id=25888
https://sourceware.org/bugzilla/attachment.cgi?id=12493
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
2020-06-29 14:09 ` Andreas Schwab
2020-06-29 14:14 ` Carlos O'Donell
@ 2020-06-29 14:16 ` Florian Weimer
1 sibling, 0 replies; 13+ messages in thread
From: Florian Weimer @ 2020-06-29 14:16 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Florian Weimer via Libc-alpha
* Andreas Schwab:
> On Jun 29 2020, Florian Weimer via Libc-alpha wrote:
>
>> * Andreas Schwab:
>>
>>> On Jun 29 2020, Florian Weimer wrote:
>>>
>>>> I think this needs barriers after and before the increments.
>>>
>>> Why doesn't gc need those barriers?
>>
>> I think it needs them as well. I thought we had them there. 8-(
>
> Should they use atomic_increment?
Yes, that as well. I don't know if the legacy atomic_increment function
implies a barrier. I think we need the equivalent of __atomic_fetch_add
(or __atomic_add_fetch) with __ATOMIC_ACQ_REL, and our <atomic.h> does
not have this, and we are not supposed to use the compiler built-ins.
atomic_fetch_add_relaxed together with atomic_full_barrier should work,
though.
Thanks,
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
@ 2020-07-09 10:41 Andreas Schwab
0 siblings, 0 replies; 13+ messages in thread
From: Andreas Schwab @ 2020-07-09 10:41 UTC (permalink / raw)
To: libc-alpha
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.
Use atomic_fetch_add to modify the GC cycle counter, including in the gc
function.
---
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);
+
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. */
--
2.26.2
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
@ 2020-09-09 14:55 Andreas Schwab
2020-09-09 18:43 ` Adhemerval Zanella
0 siblings, 1 reply; 13+ messages in thread
From: Andreas Schwab @ 2020-09-09 14:55 UTC (permalink / raw)
To: libc-alpha
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);
+
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. */
--
2.28.0
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
2020-09-09 14:55 Andreas Schwab
@ 2020-09-09 18:43 ` Adhemerval Zanella
2020-09-10 8:46 ` Andreas Schwab
0 siblings, 1 reply; 13+ messages in thread
From: Adhemerval Zanella @ 2020-09-09 18:43 UTC (permalink / raw)
To: Andreas Schwab, libc-alpha
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. */
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
2020-09-09 18:43 ` Adhemerval Zanella
@ 2020-09-10 8:46 ` Andreas Schwab
2020-09-10 9:58 ` Florian Weimer
0 siblings, 1 reply; 13+ messages in thread
From: Andreas Schwab @ 2020-09-10 8:46 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
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.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nscd: bump GC cycle during cache pruning (bug 26130)
2020-09-10 8:46 ` Andreas Schwab
@ 2020-09-10 9:58 ` Florian Weimer
0 siblings, 0 replies; 13+ messages in thread
From: Florian Weimer @ 2020-09-10 9:58 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Adhemerval Zanella, libc-alpha
* 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
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-09-10 9:58 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 14:10 [PATCH] nscd: bump GC cycle during cache pruning (bug 26130) Andreas Schwab
2020-06-29 8:27 ` Florian Weimer
2020-06-29 10:35 ` Andreas Schwab
2020-06-29 10:48 ` Florian Weimer
2020-06-29 14:09 ` Andreas Schwab
2020-06-29 14:14 ` Carlos O'Donell
2020-06-29 14:20 ` Andreas Schwab
2020-06-29 14:16 ` Florian Weimer
2020-07-09 10:41 Andreas Schwab
2020-09-09 14:55 Andreas Schwab
2020-09-09 18:43 ` Adhemerval Zanella
2020-09-10 8:46 ` Andreas Schwab
2020-09-10 9:58 ` Florian Weimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).