Re: [PATCH] memory ordering in __kfifo primitives

["Paul E. McKenney" <paulmck@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

OK, it appears that we are even.  I forgot to attach the promised
analysis of the callers to __kfifo_put() and __kfifo_get(), and
the open-iscsi@googlegroups.com email address listed as maintainer
in drivers/scsi/libiscsi.c bounces complaining that, as a non-member,
I am not allowed to send it email.  ;-)

Anyway, this time the analysis really is attached, sorry for my confusion!

Could someone please let the guys on open-iscsi@googlegroups.com know
that they should take a look at this?

						Thanx, Paul

On Wed, Aug 09, 2006 at 05:18:23PM -0700, Paul E. McKenney wrote:
> Hello!
> 
> Both __kfifo_put() and __kfifo_get() have header comments stating
> that if there is but one concurrent reader and one concurrent writer,
> locking is not necessary.  This is almost the case, but a couple of
> memory barriers are needed.  Another option would be to change the
> header comments to remove the bit about locking not being needed, and
> to change the those callers who currently don't use locking to add
> the required locking.  The attachment analyzes this approach, but the
> patch below seems simpler.
> 
> Signed-off-by: Paul E. McKenney <paulmck@us.ibm.com>
> ---
> 
>  kfifo.c |    4 ++++
>  1 files changed, 4 insertions(+)
> 
> diff -urpNa -X dontdiff linux-2.6.18-rc2/kernel/kfifo.c linux-2.6.18-rc2-kfifo/kernel/kfifo.c
> --- linux-2.6.18-rc2/kernel/kfifo.c	2006-07-15 14:53:08.000000000 -0700
> +++ linux-2.6.18-rc2-kfifo/kernel/kfifo.c	2006-08-09 14:01:53.000000000 -0700
> @@ -129,6 +129,8 @@ unsigned int __kfifo_put(struct kfifo *f
>  	/* then put the rest (if any) at the beginning of the buffer */
>  	memcpy(fifo->buffer, buffer + l, len - l);
>  
> +	smp_wmb();
> +
>  	fifo->in += len;
>  
>  	return len;
> @@ -161,6 +163,8 @@ unsigned int __kfifo_get(struct kfifo *f
>  	/* then get the rest (if any) from the beginning of the buffer */
>  	memcpy(buffer + l, fifo->buffer, len - l);
>  
> +	smp_mb();
> +
>  	fifo->out += len;
>  
>  	return len;

[-- Attachment #2: kfifo.txt --]
[-- Type: text/plain, Size: 6922 bytes --]

drivers/scsi/iscsi_tcp.c iscsi_r2t_rsp 377 rc = __kfifo_get(tcp_ctask->r2tpool.queue, (void *)&r2t, sizeof(void *));

	Covered by session->lock.

drivers/scsi/iscsi_tcp.c handle_xmstate_sol_data 1718 if (__kfifo_get(tcp_ctask->r2tqueue, (void *)&r2t, sizeof(void *))) {

	Not covered by session->lock.

drivers/scsi/iscsi_tcp.c iscsi_tcp_ctask_xmit 1839 __kfifo_get(tcp_ctask->r2tqueue, (void *)&tcp_ctask->r2t,

	Covered by session->lock.

drivers/scsi/iscsi_tcp.c iscsi_tcp_cleanup_ctask 2011 while (__kfifo_get(tcp_ctask->r2tqueue, (void *)&r2t, sizeof(void *)))

	Covered by session->lock, at least according to the comments.
	(called by iscsi_tcp_cleanup_ctask(), which is called by
	things invoking the cleanup_cmd_task function pointer, which
	is invoked by fail_command(), which has a header comment
	demanding that session->lock be held by callers.

drivers/scsi/libiscsi.c iscsi_data_xmit 556 while (__kfifo_get(conn->immqueue, (void *)&conn->mtask,

	Not covered by session->lock.

drivers/scsi/libiscsi.c iscsi_data_xmit 571 while (__kfifo_get(conn->xmitqueue, (void *)&conn->ctask,

	Not covered by session->lock.

drivers/scsi/libiscsi.c iscsi_data_xmit 590 while (__kfifo_get(conn->mgmtqueue, (void *)&conn->mtask,

	Not covered by session->lock.

drivers/scsi/libiscsi.c iscsi_data_xmit 690 __kfifo_get(session->cmdpool.queue, (void *)&ctask, sizeof(void *));

	Covered by session->lock.

drivers/scsi/libiscsi.c iscsi_conn_send_generic 766 if (!__kfifo_get(session->mgmtpool.queue,

	Covered by session->lock.

drivers/scsi/libiscsi.c iscsi_remove_task 990 __kfifo_get(fifo, (void *)&task, sizeof(void *)); \

	Macro that expands to iscsi_remove_mgmt_task() and
	iscsi_remove_cmd_task()

	o	iscsi_remove_mgmt_task called from iscsi_ctask_mtask_cleanup,
		which in turn is called from fail_command() and 
		iscsi_eh_abort().

		o	fail_command() comment indicates that session
			lock must be held.

		o	iscsi_eh_abort() has the session lock held.

	o	iscsi_remove_cmd_task() called from iscsi_eh_abort(),
		which again has the session lock held.

drivers/scsi/libiscsi.c iscsi_conn_setup 1386 if (!__kfifo_get(session->mgmtpool.queue,

	Covered by session->lock.

drivers/scsi/libiscsi.c flush_control_queues 1546 while (__kfifo_get(conn->immqueue, (void *)&mtask, sizeof(void *)) ||

	Covered by session->lock.  (Held by caller
	iscsi_start_session_recovery().)

drivers/scsi/libiscsi.c flush_control_queues 1547 __kfifo_get(conn->mgmtqueue, (void *)&mtask, sizeof(void *))) {

	Covered by session->lock.  (Held by caller
	iscsi_start_session_recovery().)

drivers/scsi/libiscsi.c fail_all_commands 1575 while (__kfifo_get(conn->xmitqueue, (void *)&ctask, sizeof(void *))) {

	Covered by session->lock.

include/linux/kfifo.h kfifo_get 113 ret = __kfifo_get(fifo, buffer, len);

	Covered by fifo->lock.

kernel/kfifo.c __kfifo_get 150 unsigned int __kfifo_get(struct kfifo *fifo,

	Covered by fifo->lock.

drivers/infiniband/ulp/iser/iser_initiator.c iser_snd_completion 674 __kfifo_put(session->mgmtpool.queue, (void *)&mtask,

	Covered by conn->session->lock.  But I have no idea what
	consumes the resulting message -- my guess is that it is
	actually going down to iscsi, based on the
	"conn = iser_conn->iscsi_conn" earlier in this function.

drivers/scsi/iscsi_tcp.c iscsi_r2t_rsp 401 __kfifo_put(tcp_ctask->r2tqueue, (void *)&r2t, sizeof(void *));

	Covered by session->lock.

drivers/scsi/iscsi_tcp.c iscsi_r2t_rsp 402 __kfifo_put(conn->xmitqueue, (void *)&ctask, sizeof(void *));

	Covered by session->lock.

drivers/scsi/iscsi_tcp.c iscsi_tcp_mtask_xmit 1388 __kfifo_put(session->mgmtpool.queue, (void *)&conn->mtask,

	Covered by session->lock.

drivers/scsi/iscsi_tcp.c handle_xmstate_sol_data 1716 __kfifo_put(tcp_ctask->r2tpool.queue, (void *)&r2t, sizeof(void *));

	Covered by session->lock.

drivers/scsi/iscsi_tcp.c iscsi_tcp_cleanup_ctask 2012 __kfifo_put(tcp_ctask->r2tpool.queue, (void *)&r2t,

	Covered by session->lock, at least according to the comments.
	(called by iscsi_tcp_cleanup_ctask(), which is called by
	things invoking the cleanup_cmd_task function pointer, which
	is invoked by fail_command(), which has a header comment
	demanding that session->lock be held by callers.

drivers/scsi/libiscsi.c iscsi_complete_command 194 __kfifo_put(session->cmdpool.queue, (void *)&ctask, sizeof(void *));

	Covered by session->lock, or at least the header comment claims
	that it should be called under this lock.

drivers/scsi/libiscsi.c __iscsi_complete_pdu 354 __kfifo_put(session->mgmtpool.queue,

	Covered by session->lock by all in-tree calls, but is an
	exported symbol.

drivers/scsi/libiscsi.c __iscsi_complete_pdu 384 __kfifo_put(session->mgmtpool.queue,

	Covered by session->lock by all in-tree calls, but is an
	exported symbol.

drivers/scsi/libiscsi.c __iscsi_complete_pdu 703 __kfifo_put(conn->xmitqueue, (void *)&ctask, sizeof(void *));

	Covered by session->lock by all in-tree calls, but is an
	exported symbol.

drivers/scsi/libiscsi.c iscsi_conn_send_generic 808 __kfifo_put(conn->immqueue, (void *)&mtask, sizeof(void *));

	Not covered by session->lock.

drivers/scsi/libiscsi.c iscsi_conn_send_generic 810 __kfifo_put(conn->mgmtqueue, (void *)&mtask, sizeof(void *));

	Not covered by session->lock.

drivers/scsi/libiscsi.c iscsi_remove_task 998 __kfifo_put(fifo, (void *)&task, sizeof(void *)); \

	Covered by session->lock, analysis is the same as for the
	earlier occurrence of iscsi_remove_task().

drivers/scsi/libiscsi.c iscsi_ctask_mtask_cleanup 1016 __kfifo_put(session->mgmtpool.queue, (void *)&ctask->mtask,

	Called from fail_command(), which has a header comment claiming
	that session->lock must be held, and from iscsi_eh_abort(),
	which has session->lock held.

drivers/scsi/libiscsi.c iscsi_eh_abort 1082 __kfifo_put(conn->xmitqueue, (void *)&pending_ctask,

	Covered by session->lock.

drivers/scsi/libiscsi.c iscsi_pool_init 1175 __kfifo_put(q->queue, (void *)&q->pool[i], sizeof(void *));

	Looks like initialization code, where no-one else would have
	a reference to the kfifo, so should be OK.

drivers/scsi/libiscsi.c iscsi_conn_setup 1406 __kfifo_put(session->mgmtpool.queue, (void *)&conn->login_mtask,

	Not covered by session->lock, despite other __kfifo operations
	being covered by this lock elsewhere in this function.

drivers/scsi/libiscsi.c iscsi_conn_teardown 1478 __kfifo_put(session->mgmtpool.queue, (void *)&conn->login_mtask,

	Covered by session->lock.

drivers/scsi/libiscsi.c flush_control_queues 1551 __kfifo_put(session->mgmtpool.queue, (void *)&mtask,

	Covered by session->lock.  (Held by caller
	iscsi_start_session_recovery().)

drivers/scsi/libiscsi.c flush_control_queues 1562 __kfifo_put(session->mgmtpool.queue, (void *)&mtask,

	Covered by session->lock.  (Held by caller
	iscsi_start_session_recovery().)

include/linux/kfifo.h kfifo_put 89 ret = __kfifo_put(fifo, buffer, len);

	Covered by fifo->lock.