[lttng-dev] [PATCH lttng-modules] Fix: ACCESS_ONCE() removed in kernel 4.15

Mathieu Desnoyers mathieu.desnoyers at efficios.com
Tue Dec 19 20:45:34 UTC 2017


----- On Dec 19, 2017, at 3:41 PM, Michael Jeanson mjeanson at efficios.com wrote:

> On 2017-12-19 15:32, Mathieu Desnoyers wrote:
>> ----- On Dec 19, 2017, at 3:06 PM, Michael Jeanson mjeanson at efficios.com wrote:
>> 
>>> The ACCESS_ONCE() macro was removed in kernel 4.15 and should be
>>> replaced by READ_ONCE and WRITE_ONCE which were introduced in kernel
>>> 3.19.
>>>
>>> This commit replaces all calls to ACCESS_ONCE() with the appropriate
>>> READ_ONCE or WRITE_ONCE and adds compatbility macros for kernels that
>>> have them.
>>>
>>> See this upstream commit:
>>>
>>>  commit b03a0fe0c5e4b46dcd400d27395b124499554a71
>>>  Author: Paul E. McKenney <paulmck at linux.vnet.ibm.com>
>>>  Date:   Mon Oct 23 14:07:25 2017 -0700
>>>
>>>    locking/atomics, mm: Convert ACCESS_ONCE() to READ_ONCE()/WRITE_ONCE()
>>>
>>>    For several reasons, it is desirable to use {READ,WRITE}_ONCE() in
>>>    preference to ACCESS_ONCE(), and new code is expected to use one of the
>>>    former. So far, there's been no reason to change most existing uses of
>>>    ACCESS_ONCE(), as these aren't currently harmful.
>>>
>>>    However, for some features it is necessary to instrument reads and
>>>    writes separately, which is not possible with ACCESS_ONCE(). This
>>>    distinction is critical to correct operation.
>>>
>>>    It's possible to transform the bulk of kernel code using the Coccinelle
>>>    script below. However, this doesn't handle comments, leaving references
>>>    to ACCESS_ONCE() instances which have been removed. As a preparatory
>>>    step, this patch converts the mm code and comments to use
>>>    {READ,WRITE}_ONCE() consistently.
>>>
>>>    ----
>>>    virtual patch
>>>
>>>    @ depends on patch @
>>>    expression E1, E2;
>>>    @@
>>>
>>>    - ACCESS_ONCE(E1) = E2
>>>    + WRITE_ONCE(E1, E2)
>>>
>>>    @ depends on patch @
>>>    expression E;
>>>    @@
>>>
>>>    - ACCESS_ONCE(E)
>>>    + READ_ONCE(E)
>>>    ----
>>>
>>> Signed-off-by: Michael Jeanson <mjeanson at efficios.com>
>>> ---
>>> instrumentation/events/lttng-module/i2c.h |  4 ++--
>>> lib/ringbuffer/backend.h                  |  2 +-
>>> lib/ringbuffer/backend_internal.h         |  7 ++++---
>>> lib/ringbuffer/frontend.h                 |  2 +-
>>> lib/ringbuffer/ring_buffer_frontend.c     | 10 +++++-----
>>> lib/ringbuffer/ring_buffer_iterator.c     |  2 +-
>>> lttng-clock.c                             |  6 +++---
>>> lttng-events.c                            | 30 +++++++++++++++---------------
>>> probes/lttng-ftrace.c                     | 12 ++++++------
>>> probes/lttng-kprobes.c                    |  6 +++---
>>> probes/lttng-kretprobes.c                 | 10 +++++-----
>>> probes/lttng-tracepoint-event-impl.h      | 12 ++++++------
>>> wrapper/compiler.h                        | 13 +++++++++++++
>>> wrapper/trace-clock.h                     | 11 ++++++-----
>>> 14 files changed, 71 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/instrumentation/events/lttng-module/i2c.h
>>> b/instrumentation/events/lttng-module/i2c.h
>>> index 4088b60..dd91c9b 100644
>>> --- a/instrumentation/events/lttng-module/i2c.h
>>> +++ b/instrumentation/events/lttng-module/i2c.h
>>> @@ -22,7 +22,7 @@ LTTNG_TRACEPOINT_EVENT_CODE(i2c_write,
>>>
>>> 	TP_code_pre(
>>> 		tp_locvar->extract_sensitive_payload =
>>> -			ACCESS_ONCE(extract_sensitive_payload);
>>> +			READ_ONCE(extract_sensitive_payload);
>>> 	),
>>>
>>> 	TP_FIELDS(
>>> @@ -77,7 +77,7 @@ LTTNG_TRACEPOINT_EVENT_CODE(i2c_reply,
>>>
>>> 	TP_code_pre(
>>> 		tp_locvar->extract_sensitive_payload =
>>> -			ACCESS_ONCE(extract_sensitive_payload);
>>> +			READ_ONCE(extract_sensitive_payload);
>>> 	),
>>>
>>> 	TP_FIELDS(
>>> diff --git a/lib/ringbuffer/backend.h b/lib/ringbuffer/backend.h
>>> index 9db0095..0b75de8 100644
>>> --- a/lib/ringbuffer/backend.h
>>> +++ b/lib/ringbuffer/backend.h
>>> @@ -169,7 +169,7 @@ size_t lib_ring_buffer_do_strcpy(const struct
>>> lib_ring_buffer_config *config,
>>> 		 * Only read source character once, in case it is
>>> 		 * modified concurrently.
>>> 		 */
>>> -		c = ACCESS_ONCE(src[count]);
>>> +		c = READ_ONCE(src[count]);
>>> 		if (!c)
>>> 			break;
>>> 		lib_ring_buffer_do_copy(config, &dest[count], &c, 1);
>>> diff --git a/lib/ringbuffer/backend_internal.h
>>> b/lib/ringbuffer/backend_internal.h
>>> index 2e59b68..dc69ecf 100644
>>> --- a/lib/ringbuffer/backend_internal.h
>>> +++ b/lib/ringbuffer/backend_internal.h
>>> @@ -23,6 +23,7 @@
>>>  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>>  */
>>>
>>> +#include <wrapper/compiler.h>
>>> #include <wrapper/ringbuffer/config.h>
>>> #include <wrapper/ringbuffer/backend_types.h>
>>> #include <wrapper/ringbuffer/frontend_types.h>
>>> @@ -171,7 +172,7 @@ void subbuffer_id_set_noref_offset(const struct
>>> lib_ring_buffer_config *config,
>>> 		tmp |= offset << SB_ID_OFFSET_SHIFT;
>>> 		tmp |= SB_ID_NOREF_MASK;
>>> 		/* Volatile store, read concurrently by readers. */
>>> -		ACCESS_ONCE(*id) = tmp;
>>> +		WRITE_ONCE(*id, tmp);
>>> 	}
>>> }
>>>
>>> @@ -379,7 +380,7 @@ void lib_ring_buffer_clear_noref(const struct
>>> lib_ring_buffer_config *config,
>>> 	 * Performing a volatile access to read the sb_pages, because we want to
>>> 	 * read a coherent version of the pointer and the associated noref flag.
>>> 	 */
>>> -	id = ACCESS_ONCE(bufb->buf_wsb[idx].id);
>>> +	id = READ_ONCE(bufb->buf_wsb[idx].id);
>>> 	for (;;) {
>>> 		/* This check is called on the fast path for each record. */
>>> 		if (likely(!subbuffer_id_is_noref(config, id))) {
>>> @@ -448,7 +449,7 @@ int update_read_sb_index(const struct lib_ring_buffer_config
>>> *config,
>>> 	if (config->mode == RING_BUFFER_OVERWRITE) {
>>> 		/*
>>> 		 * Exchange the target writer subbuffer with our own unused
>>> -		 * subbuffer. No need to use ACCESS_ONCE() here to read the
>>> +		 * subbuffer. No need to use READ_ONCE() here to read the
>>> 		 * old_wpage, because the value read will be confirmed by the
>>> 		 * following cmpxchg().
>>> 		 */
>>> diff --git a/lib/ringbuffer/frontend.h b/lib/ringbuffer/frontend.h
>>> index 909abc2..1450cb7 100644
>>> --- a/lib/ringbuffer/frontend.h
>>> +++ b/lib/ringbuffer/frontend.h
>>> @@ -168,7 +168,7 @@ static inline
>>> int lib_ring_buffer_is_finalized(const struct lib_ring_buffer_config *config,
>>> 				 struct lib_ring_buffer *buf)
>>> {
>>> -	int finalized = ACCESS_ONCE(buf->finalized);
>>> +	int finalized = READ_ONCE(buf->finalized);
>>> 	/*
>>> 	 * Read finalized before counters.
>>> 	 */
>>> diff --git a/lib/ringbuffer/ring_buffer_frontend.c
>>> b/lib/ringbuffer/ring_buffer_frontend.c
>>> index abd9757..0d8279b 100644
>>> --- a/lib/ringbuffer/ring_buffer_frontend.c
>>> +++ b/lib/ringbuffer/ring_buffer_frontend.c
>>> @@ -983,7 +983,7 @@ void *channel_destroy(struct channel *chan)
>>> 			 * Perform flush before writing to finalized.
>>> 			 */
>>> 			smp_wmb();
>>> -			ACCESS_ONCE(buf->finalized) = 1;
>>> +			WRITE_ONCE(buf->finalized, 1);
>>> 			wake_up_interruptible(&buf->read_wait);
>>> 		}
>>> 	} else {
>>> @@ -997,10 +997,10 @@ void *channel_destroy(struct channel *chan)
>>> 		 * Perform flush before writing to finalized.
>>> 		 */
>>> 		smp_wmb();
>>> -		ACCESS_ONCE(buf->finalized) = 1;
>>> +		WRITE_ONCE(buf->finalized, 1);
>>> 		wake_up_interruptible(&buf->read_wait);
>>> 	}
>>> -	ACCESS_ONCE(chan->finalized) = 1;
>>> +	WRITE_ONCE(chan->finalized, 1);
>>> 	wake_up_interruptible(&chan->hp_wait);
>>> 	wake_up_interruptible(&chan->read_wait);
>>> 	priv = chan->backend.priv;
>>> @@ -1077,7 +1077,7 @@ int lib_ring_buffer_snapshot(struct lib_ring_buffer *buf,
>>> 	int finalized;
>>>
>>> retry:
>>> -	finalized = ACCESS_ONCE(buf->finalized);
>>> +	finalized = READ_ONCE(buf->finalized);
>>> 	/*
>>> 	 * Read finalized before counters.
>>> 	 */
>>> @@ -1248,7 +1248,7 @@ int lib_ring_buffer_get_subbuf(struct lib_ring_buffer
>>> *buf,
>>> 		return -EBUSY;
>>> 	}
>>> retry:
>>> -	finalized = ACCESS_ONCE(buf->finalized);
>>> +	finalized = READ_ONCE(buf->finalized);
>>> 	/*
>>> 	 * Read finalized before counters.
>>> 	 */
>>> diff --git a/lib/ringbuffer/ring_buffer_iterator.c
>>> b/lib/ringbuffer/ring_buffer_iterator.c
>>> index b6bec48..61eaa5b 100644
>>> --- a/lib/ringbuffer/ring_buffer_iterator.c
>>> +++ b/lib/ringbuffer/ring_buffer_iterator.c
>>> @@ -61,7 +61,7 @@ restart:
>>> 	switch (iter->state) {
>>> 	case ITER_GET_SUBBUF:
>>> 		ret = lib_ring_buffer_get_next_subbuf(buf);
>>> -		if (ret && !ACCESS_ONCE(buf->finalized)
>>> +		if (ret && !READ_ONCE(buf->finalized)
>>> 		    && config->alloc == RING_BUFFER_ALLOC_GLOBAL) {
>>> 			/*
>>> 			 * Use "pull" scheme for global buffers. The reader
>>> diff --git a/lttng-clock.c b/lttng-clock.c
>>> index a5a7eaa..48b4be5 100644
>>> --- a/lttng-clock.c
>>> +++ b/lttng-clock.c
>>> @@ -48,7 +48,7 @@ int lttng_clock_register_plugin(struct lttng_trace_clock *ltc,
>>> 		goto end;
>>> 	}
>>> 	/* set clock */
>>> -	ACCESS_ONCE(lttng_trace_clock) = ltc;
>>> +	WRITE_ONCE(lttng_trace_clock, ltc);
>>> 	lttng_trace_clock_mod = mod;
>>> end:
>>> 	mutex_unlock(&clock_mutex);
>>> @@ -66,7 +66,7 @@ void lttng_clock_unregister_plugin(struct lttng_trace_clock
>>> *ltc,
>>> 	}
>>> 	WARN_ON_ONCE(lttng_trace_clock_mod != mod);
>>>
>>> -	ACCESS_ONCE(lttng_trace_clock) = NULL;
>>> +	WRITE_ONCE(lttng_trace_clock, NULL);
>>> 	lttng_trace_clock_mod = NULL;
>>> end:
>>> 	mutex_unlock(&clock_mutex);
>>> @@ -83,7 +83,7 @@ void lttng_clock_ref(void)
>>> 		ret = try_module_get(lttng_trace_clock_mod);
>>> 		if (!ret) {
>>> 			printk(KERN_ERR "LTTng-clock cannot get clock plugin module\n");
>>> -			ACCESS_ONCE(lttng_trace_clock) = NULL;
>>> +			WRITE_ONCE(lttng_trace_clock, NULL);
>>> 			lttng_trace_clock_mod = NULL;
>>> 		}
>>> 	}
>>> diff --git a/lttng-events.c b/lttng-events.c
>>> index 21c4113..75c3fb1 100644
>>> --- a/lttng-events.c
>>> +++ b/lttng-events.c
>>> @@ -186,7 +186,7 @@ void lttng_session_destroy(struct lttng_session *session)
>>> 	int ret;
>>>
>>> 	mutex_lock(&sessions_mutex);
>>> -	ACCESS_ONCE(session->active) = 0;
>>> +	WRITE_ONCE(session->active, 0);
>>> 	list_for_each_entry(chan, &session->chan, list) {
>>> 		ret = lttng_syscalls_unregister(chan);
>>> 		WARN_ON(ret);
>>> @@ -261,16 +261,16 @@ int lttng_session_enable(struct lttng_session *session)
>>> 			lib_ring_buffer_clear_quiescent_channel(chan->chan);
>>> 	}
>>>
>>> -	ACCESS_ONCE(session->active) = 1;
>>> -	ACCESS_ONCE(session->been_active) = 1;
>>> +	WRITE_ONCE(session->active, 1);
>>> +	WRITE_ONCE(session->been_active, 1);
>>> 	ret = _lttng_session_metadata_statedump(session);
>>> 	if (ret) {
>>> -		ACCESS_ONCE(session->active) = 0;
>>> +		WRITE_ONCE(session->active, 0);
>>> 		goto end;
>>> 	}
>>> 	ret = lttng_statedump_start(session);
>>> 	if (ret)
>>> -		ACCESS_ONCE(session->active) = 0;
>>> +		WRITE_ONCE(session->active, 0);
>>> end:
>>> 	mutex_unlock(&sessions_mutex);
>>> 	return ret;
>>> @@ -286,7 +286,7 @@ int lttng_session_disable(struct lttng_session *session)
>>> 		ret = -EBUSY;
>>> 		goto end;
>>> 	}
>>> -	ACCESS_ONCE(session->active) = 0;
>>> +	WRITE_ONCE(session->active, 0);
>>>
>>> 	/* Set transient enabler state to "disabled" */
>>> 	session->tstate = 0;
>>> @@ -361,7 +361,7 @@ int lttng_channel_enable(struct lttng_channel *channel)
>>> 	channel->tstate = 1;
>>> 	lttng_session_sync_enablers(channel->session);
>>> 	/* Set atomically the state to "enabled" */
>>> -	ACCESS_ONCE(channel->enabled) = 1;
>>> +	WRITE_ONCE(channel->enabled, 1);
>>> end:
>>> 	mutex_unlock(&sessions_mutex);
>>> 	return ret;
>>> @@ -381,7 +381,7 @@ int lttng_channel_disable(struct lttng_channel *channel)
>>> 		goto end;
>>> 	}
>>> 	/* Set atomically the state to "disabled" */
>>> -	ACCESS_ONCE(channel->enabled) = 0;
>>> +	WRITE_ONCE(channel->enabled, 0);
>>> 	/* Set transient enabler state to "enabled" */
>>> 	channel->tstate = 0;
>>> 	lttng_session_sync_enablers(channel->session);
>>> @@ -411,7 +411,7 @@ int lttng_event_enable(struct lttng_event *event)
>>> 	case LTTNG_KERNEL_KPROBE:
>>> 	case LTTNG_KERNEL_FUNCTION:
>>> 	case LTTNG_KERNEL_NOOP:
>>> -		ACCESS_ONCE(event->enabled) = 1;
>>> +		WRITE_ONCE(event->enabled, 1);
>>> 		break;
>>> 	case LTTNG_KERNEL_KRETPROBE:
>>> 		ret = lttng_kretprobes_event_enable_state(event, 1);
>>> @@ -446,7 +446,7 @@ int lttng_event_disable(struct lttng_event *event)
>>> 	case LTTNG_KERNEL_KPROBE:
>>> 	case LTTNG_KERNEL_FUNCTION:
>>> 	case LTTNG_KERNEL_NOOP:
>>> -		ACCESS_ONCE(event->enabled) = 0;
>>> +		WRITE_ONCE(event->enabled, 0);
>>> 		break;
>>> 	case LTTNG_KERNEL_KRETPROBE:
>>> 		ret = lttng_kretprobes_event_enable_state(event, 0);
>>> @@ -1517,7 +1517,7 @@ void lttng_session_sync_enablers(struct lttng_session
>>> *session)
>>> 		 */
>>> 		enabled = enabled && session->tstate && event->chan->tstate;
>>>
>>> -		ACCESS_ONCE(event->enabled) = enabled;
>>> +		WRITE_ONCE(event->enabled, enabled);
>>> 		/*
>>> 		 * Sync tracepoint registration with event enabled
>>> 		 * state.
>>> @@ -1643,7 +1643,7 @@ int lttng_metadata_printf(struct lttng_session *session,
>>> 	va_list ap;
>>> 	struct lttng_metadata_stream *stream;
>>>
>>> -	WARN_ON_ONCE(!ACCESS_ONCE(session->active));
>>> +	WARN_ON_ONCE(!READ_ONCE(session->active));
>>>
>>> 	va_start(ap, fmt);
>>> 	str = kvasprintf(GFP_KERNEL, fmt, ap);
>>> @@ -2230,7 +2230,7 @@ int _lttng_event_metadata_statedump(struct lttng_session
>>> *session,
>>> {
>>> 	int ret = 0;
>>>
>>> -	if (event->metadata_dumped || !ACCESS_ONCE(session->active))
>>> +	if (event->metadata_dumped || !READ_ONCE(session->active))
>>> 		return 0;
>>> 	if (chan->channel_type == METADATA_CHANNEL)
>>> 		return 0;
>>> @@ -2297,7 +2297,7 @@ int _lttng_channel_metadata_statedump(struct lttng_session
>>> *session,
>>> {
>>> 	int ret = 0;
>>>
>>> -	if (chan->metadata_dumped || !ACCESS_ONCE(session->active))
>>> +	if (chan->metadata_dumped || !READ_ONCE(session->active))
>>> 		return 0;
>>>
>>> 	if (chan->channel_type == METADATA_CHANNEL)
>>> @@ -2454,7 +2454,7 @@ int _lttng_session_metadata_statedump(struct lttng_session
>>> *session)
>>> 	struct lttng_event *event;
>>> 	int ret = 0;
>>>
>>> -	if (!ACCESS_ONCE(session->active))
>>> +	if (!READ_ONCE(session->active))
>>> 		return 0;
>>> 	if (session->metadata_dumped)
>>> 		goto skip_session;
>>> diff --git a/probes/lttng-ftrace.c b/probes/lttng-ftrace.c
>>> index 9ec326e..50675a4 100644
>>> --- a/probes/lttng-ftrace.c
>>> +++ b/probes/lttng-ftrace.c
>>> @@ -58,11 +58,11 @@ void lttng_ftrace_handler(unsigned long ip, unsigned long
>>> parent_ip,
>>> 	} payload;
>>> 	int ret;
>>>
>>> -	if (unlikely(!ACCESS_ONCE(chan->session->active)))
>>> +	if (unlikely(!READ_ONCE(chan->session->active)))
>>> 		return;
>>> -	if (unlikely(!ACCESS_ONCE(chan->enabled)))
>>> +	if (unlikely(!READ_ONCE(chan->enabled)))
>>> 		return;
>>> -	if (unlikely(!ACCESS_ONCE(event->enabled)))
>>> +	if (unlikely(!READ_ONCE(event->enabled)))
>>> 		return;
>>>
>>> 	lib_ring_buffer_ctx_init(&ctx, chan->chan, &lttng_probe_ctx,
>>> @@ -94,11 +94,11 @@ void lttng_ftrace_handler(unsigned long ip, unsigned long
>>> parent_ip, void **data
>>> 	} payload;
>>> 	int ret;
>>>
>>> -	if (unlikely(!ACCESS_ONCE(chan->session->active)))
>>> +	if (unlikely(!READ_ONCE(chan->session->active)))
>>> 		return;
>>> -	if (unlikely(!ACCESS_ONCE(chan->enabled)))
>>> +	if (unlikely(!READ_ONCE(chan->enabled)))
>>> 		return;
>>> -	if (unlikely(!ACCESS_ONCE(event->enabled)))
>>> +	if (unlikely(!READ_ONCE(event->enabled)))
>>> 		return;
>>>
>>> 	lib_ring_buffer_ctx_init(&ctx, chan->chan, &lttng_probe_ctx,
>>> diff --git a/probes/lttng-kprobes.c b/probes/lttng-kprobes.c
>>> index daf14ce..b58a09b 100644
>>> --- a/probes/lttng-kprobes.c
>>> +++ b/probes/lttng-kprobes.c
>>> @@ -43,11 +43,11 @@ int lttng_kprobes_handler_pre(struct kprobe *p, struct
>>> pt_regs *regs)
>>> 	int ret;
>>> 	unsigned long data = (unsigned long) p->addr;
>>>
>>> -	if (unlikely(!ACCESS_ONCE(chan->session->active)))
>>> +	if (unlikely(!READ_ONCE(chan->session->active)))
>>> 		return 0;
>>> -	if (unlikely(!ACCESS_ONCE(chan->enabled)))
>>> +	if (unlikely(!READ_ONCE(chan->enabled)))
>>> 		return 0;
>>> -	if (unlikely(!ACCESS_ONCE(event->enabled)))
>>> +	if (unlikely(!READ_ONCE(event->enabled)))
>>> 		return 0;
>>>
>>> 	lib_ring_buffer_ctx_init(&ctx, chan->chan, &lttng_probe_ctx, sizeof(data),
>>> diff --git a/probes/lttng-kretprobes.c b/probes/lttng-kretprobes.c
>>> index 498df62..49b7de8 100644
>>> --- a/probes/lttng-kretprobes.c
>>> +++ b/probes/lttng-kretprobes.c
>>> @@ -63,11 +63,11 @@ int _lttng_kretprobes_handler(struct kretprobe_instance
>>> *krpi,
>>> 		unsigned long parent_ip;
>>> 	} payload;
>>>
>>> -	if (unlikely(!ACCESS_ONCE(chan->session->active)))
>>> +	if (unlikely(!READ_ONCE(chan->session->active)))
>>> 		return 0;
>>> -	if (unlikely(!ACCESS_ONCE(chan->enabled)))
>>> +	if (unlikely(!READ_ONCE(chan->enabled)))
>>> 		return 0;
>>> -	if (unlikely(!ACCESS_ONCE(event->enabled)))
>>> +	if (unlikely(!READ_ONCE(event->enabled)))
>>> 		return 0;
>>>
>>> 	payload.ip = (unsigned long) krpi->rp->kp.addr;
>>> @@ -304,8 +304,8 @@ int lttng_kretprobes_event_enable_state(struct lttng_event
>>> *event,
>>> 	}
>>> 	lttng_krp = event->u.kretprobe.lttng_krp;
>>> 	event_return = lttng_krp->event[EVENT_RETURN];
>>> -	ACCESS_ONCE(event->enabled) = enable;
>>> -	ACCESS_ONCE(event_return->enabled) = enable;
>>> +	WRITE_ONCE(event->enabled, enable);
>>> +	WRITE_ONCE(event_return->enabled, enable);
>>> 	return 0;
>>> }
>>> EXPORT_SYMBOL_GPL(lttng_kretprobes_event_enable_state);
>>> diff --git a/probes/lttng-tracepoint-event-impl.h
>>> b/probes/lttng-tracepoint-event-impl.h
>>> index 7ec0d75..61f1c2d 100644
>>> --- a/probes/lttng-tracepoint-event-impl.h
>>> +++ b/probes/lttng-tracepoint-event-impl.h
>>> @@ -1143,11 +1143,11 @@ static void __event_probe__##_name(void *__data, _proto)
>>> 		      \
>>> 									      \
>>> 	if (!_TP_SESSION_CHECK(session, __session))			      \
>>> 		return;							      \
>>> -	if (unlikely(!ACCESS_ONCE(__session->active)))			      \
>>> +	if (unlikely(!READ_ONCE(__session->active)))			      \
>>> 		return;							      \
>>> -	if (unlikely(!ACCESS_ONCE(__chan->enabled)))			      \
>>> +	if (unlikely(!READ_ONCE(__chan->enabled)))			      \
>>> 		return;							      \
>>> -	if (unlikely(!ACCESS_ONCE(__event->enabled)))			      \
>>> +	if (unlikely(!READ_ONCE(__event->enabled)))			      \
>>> 		return;							      \
>>> 	__lpf = lttng_rcu_dereference(__session->pid_tracker);		      \
>>> 	if (__lpf && likely(!lttng_pid_tracker_lookup(__lpf, current->tgid))) \
>>> @@ -1217,11 +1217,11 @@ static void __event_probe__##_name(void *__data)
>>> 			      \
>>> 									      \
>>> 	if (!_TP_SESSION_CHECK(session, __session))			      \
>>> 		return;							      \
>>> -	if (unlikely(!ACCESS_ONCE(__session->active)))			      \
>>> +	if (unlikely(!READ_ONCE(__session->active)))			      \
>>> 		return;							      \
>>> -	if (unlikely(!ACCESS_ONCE(__chan->enabled)))			      \
>>> +	if (unlikely(!READ_ONCE(__chan->enabled)))			      \
>>> 		return;							      \
>>> -	if (unlikely(!ACCESS_ONCE(__event->enabled)))			      \
>>> +	if (unlikely(!READ_ONCE(__event->enabled)))			      \
>>> 		return;							      \
>>> 	__lpf = lttng_rcu_dereference(__session->pid_tracker);		      \
>>> 	if (__lpf && likely(!lttng_pid_tracker_lookup(__lpf, current->pid)))  \
>>> diff --git a/wrapper/compiler.h b/wrapper/compiler.h
>>> index 0c01632..060698f 100644
>>> --- a/wrapper/compiler.h
>>> +++ b/wrapper/compiler.h
>>> @@ -39,4 +39,17 @@
>>> # endif
>>> #endif
>>>
>>> +/*
>>> + * READ/WRITE_ONCE were introduced in kernel 3.19 and ACCESS_ONCE
>>> + * was removed in 4.15. Prefer READ/WRITE but fallback to ACCESS
>>> + * when they are not available.
>>> + */
>>> +#ifndef READ_ONCE
>>> +# define READ_ONCE(x) ACCESS_ONCE(x)
>>> +#endif
>>> +
>>> +#ifndef WRITE_ONCE
>>> +# define WRITE_ONCE(x, val) ACCESS_ONCE(x) = val
>> 
>> I think it would be safer to use a statement-expression for this one:
>> 
>> # define WRITE_ONCE(x, val) ({ ACCESS_ONCE(x) = val; })
>> 
>> Given that WRITE_ONCE and ACCESS_ONCE evaluate to the result of the assignment,
>> someone could legitimately do e.g.:
>> 
>> int *myvar = *WRITE_ONCE(x, newptr);
>> 
>> Where the "*" would be applied to the ACCESS_ONCE() rather than the evaluated
>> value.
>> 
>> Makes sense ?
> 
> Sure, you can merge the modified version or do you prefer a v2 patch?

I can do it on my side, thanks!

Mathieu

> 
>> 
>> Thanks,
>> 
>> Mathieu
>> 
>>> +#endif
>>> +
>>> #endif /* _LTTNG_WRAPPER_COMPILER_H */
>>> diff --git a/wrapper/trace-clock.h b/wrapper/trace-clock.h
>>> index 7f17ccd..08f9922 100644
>>> --- a/wrapper/trace-clock.h
>>> +++ b/wrapper/trace-clock.h
>>> @@ -37,6 +37,7 @@
>>> #include <asm/local.h>
>>> #include <lttng-kernel-version.h>
>>> #include <lttng-clock.h>
>>> +#include <wrapper/compiler.h>
>>> #include <wrapper/percpu-defs.h>
>>> #include <wrapper/random.h>
>>>
>>> @@ -176,7 +177,7 @@ static inline void put_trace_clock(void)
>>>
>>> static inline u64 trace_clock_read64(void)
>>> {
>>> -	struct lttng_trace_clock *ltc = ACCESS_ONCE(lttng_trace_clock);
>>> +	struct lttng_trace_clock *ltc = READ_ONCE(lttng_trace_clock);
>>>
>>> 	if (likely(!ltc)) {
>>> 		return trace_clock_read64_monotonic();
>>> @@ -188,7 +189,7 @@ static inline u64 trace_clock_read64(void)
>>>
>>> static inline u64 trace_clock_freq(void)
>>> {
>>> -	struct lttng_trace_clock *ltc = ACCESS_ONCE(lttng_trace_clock);
>>> +	struct lttng_trace_clock *ltc = READ_ONCE(lttng_trace_clock);
>>>
>>> 	if (!ltc) {
>>> 		return trace_clock_freq_monotonic();
>>> @@ -200,7 +201,7 @@ static inline u64 trace_clock_freq(void)
>>>
>>> static inline int trace_clock_uuid(char *uuid)
>>> {
>>> -	struct lttng_trace_clock *ltc = ACCESS_ONCE(lttng_trace_clock);
>>> +	struct lttng_trace_clock *ltc = READ_ONCE(lttng_trace_clock);
>>>
>>> 	read_barrier_depends();	/* load ltc before content */
>>> 	/* Use default UUID cb when NULL */
>>> @@ -213,7 +214,7 @@ static inline int trace_clock_uuid(char *uuid)
>>>
>>> static inline const char *trace_clock_name(void)
>>> {
>>> -	struct lttng_trace_clock *ltc = ACCESS_ONCE(lttng_trace_clock);
>>> +	struct lttng_trace_clock *ltc = READ_ONCE(lttng_trace_clock);
>>>
>>> 	if (!ltc) {
>>> 		return trace_clock_name_monotonic();
>>> @@ -225,7 +226,7 @@ static inline const char *trace_clock_name(void)
>>>
>>> static inline const char *trace_clock_description(void)
>>> {
>>> -	struct lttng_trace_clock *ltc = ACCESS_ONCE(lttng_trace_clock);
>>> +	struct lttng_trace_clock *ltc = READ_ONCE(lttng_trace_clock);
>>>
>>> 	if (!ltc) {
>>> 		return trace_clock_description_monotonic();
>>> --
>>> 2.7.4

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com


More information about the lttng-dev mailing list