| Message ID | 20241029191425.2519085-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a911bad094b010e276f072fe9a599b66e59ed5fe |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] dql: annotate data-races around dql->last_obj_cnt | expand |
On Tue, Oct 29, 2024 at 07:14:25PM +0000, Eric Dumazet wrote: > dql->last_obj_cnt is read/written from different contexts, > without any lock synchronization. > > Use READ_ONCE()/WRITE_ONCE() to avoid load/store tearing. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > include/linux/dynamic_queue_limits.h | 2 +- > lib/dynamic_queue_limits.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h > index 281298e77a1579cba1f92a3b3f03b8be089fd38f..808b1a5102e7c0bbbcd9676b0dacadad2f0ee49a 100644 > --- a/include/linux/dynamic_queue_limits.h > +++ b/include/linux/dynamic_queue_limits.h > @@ -127,7 +127,7 @@ static inline void dql_queued(struct dql *dql, unsigned int count) > if (WARN_ON_ONCE(count > DQL_MAX_OBJECT)) > return; > > - dql->last_obj_cnt = count; > + WRITE_ONCE(dql->last_obj_cnt, count); > > /* We want to force a write first, so that cpu do not attempt > * to get cache line containing last_obj_cnt, num_queued, adj_limit > diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c > index e49deddd3de9fe9e98d6712559cf48d12a0a2537..c1b7638a594ac43f947e00decabbd3468dcb53de 100644 > --- a/lib/dynamic_queue_limits.c > +++ b/lib/dynamic_queue_limits.c > @@ -179,7 +179,7 @@ void dql_completed(struct dql *dql, unsigned int count) > > dql->adj_limit = limit + completed; > dql->prev_ovlimit = ovlimit; > - dql->prev_last_obj_cnt = dql->last_obj_cnt; > + dql->prev_last_obj_cnt = READ_ONCE(dql->last_obj_cnt); > dql->num_completed = completed; > dql->prev_num_queued = num_queued; > This looks fine to me. I noted that dql_reset writes last_obj_cnt, but AFAIU that write is not a problem (from the 1 driver I looked at). Reviewed-by: Joe Damato <jdamato@fastly.com>
On Tue, Oct 29, 2024 at 8:34 PM Joe Damato <jdamato@fastly.com> wrote: > > On Tue, Oct 29, 2024 at 07:14:25PM +0000, Eric Dumazet wrote: > > dql->last_obj_cnt is read/written from different contexts, > > without any lock synchronization. > > > > Use READ_ONCE()/WRITE_ONCE() to avoid load/store tearing. > > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > --- > > include/linux/dynamic_queue_limits.h | 2 +- > > lib/dynamic_queue_limits.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h > > index 281298e77a1579cba1f92a3b3f03b8be089fd38f..808b1a5102e7c0bbbcd9676b0dacadad2f0ee49a 100644 > > --- a/include/linux/dynamic_queue_limits.h > > +++ b/include/linux/dynamic_queue_limits.h > > @@ -127,7 +127,7 @@ static inline void dql_queued(struct dql *dql, unsigned int count) > > if (WARN_ON_ONCE(count > DQL_MAX_OBJECT)) > > return; > > > > - dql->last_obj_cnt = count; > > + WRITE_ONCE(dql->last_obj_cnt, count); > > > > /* We want to force a write first, so that cpu do not attempt > > * to get cache line containing last_obj_cnt, num_queued, adj_limit > > diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c > > index e49deddd3de9fe9e98d6712559cf48d12a0a2537..c1b7638a594ac43f947e00decabbd3468dcb53de 100644 > > --- a/lib/dynamic_queue_limits.c > > +++ b/lib/dynamic_queue_limits.c > > @@ -179,7 +179,7 @@ void dql_completed(struct dql *dql, unsigned int count) > > > > dql->adj_limit = limit + completed; > > dql->prev_ovlimit = ovlimit; > > - dql->prev_last_obj_cnt = dql->last_obj_cnt; > > + dql->prev_last_obj_cnt = READ_ONCE(dql->last_obj_cnt); > > dql->num_completed = completed; > > dql->prev_num_queued = num_queued; > > > > This looks fine to me. I noted that dql_reset writes last_obj_cnt, > but AFAIU that write is not a problem (from the 1 driver I looked > at). Yeah, I think that dql_reset() should not exist in the first place. When all skbs are properly tx completed, BQL state should be known and clean.
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 29 Oct 2024 19:14:25 +0000 you wrote: > dql->last_obj_cnt is read/written from different contexts, > without any lock synchronization. > > Use READ_ONCE()/WRITE_ONCE() to avoid load/store tearing. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > [...] Here is the summary with links: - [net-next] dql: annotate data-races around dql->last_obj_cnt https://git.kernel.org/netdev/net-next/c/a911bad094b0 You are awesome, thank you!
diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h index 281298e77a1579cba1f92a3b3f03b8be089fd38f..808b1a5102e7c0bbbcd9676b0dacadad2f0ee49a 100644 --- a/include/linux/dynamic_queue_limits.h +++ b/include/linux/dynamic_queue_limits.h @@ -127,7 +127,7 @@ static inline void dql_queued(struct dql *dql, unsigned int count) if (WARN_ON_ONCE(count > DQL_MAX_OBJECT)) return; - dql->last_obj_cnt = count; + WRITE_ONCE(dql->last_obj_cnt, count); /* We want to force a write first, so that cpu do not attempt * to get cache line containing last_obj_cnt, num_queued, adj_limit diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c index e49deddd3de9fe9e98d6712559cf48d12a0a2537..c1b7638a594ac43f947e00decabbd3468dcb53de 100644 --- a/lib/dynamic_queue_limits.c +++ b/lib/dynamic_queue_limits.c @@ -179,7 +179,7 @@ void dql_completed(struct dql *dql, unsigned int count) dql->adj_limit = limit + completed; dql->prev_ovlimit = ovlimit; - dql->prev_last_obj_cnt = dql->last_obj_cnt; + dql->prev_last_obj_cnt = READ_ONCE(dql->last_obj_cnt); dql->num_completed = completed; dql->prev_num_queued = num_queued;
dql->last_obj_cnt is read/written from different contexts, without any lock synchronization. Use READ_ONCE()/WRITE_ONCE() to avoid load/store tearing. Signed-off-by: Eric Dumazet <edumazet@google.com> --- include/linux/dynamic_queue_limits.h | 2 +- lib/dynamic_queue_limits.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)