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 [email protected] 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 [email protected] 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 <[email protected]>
> ---
>
> 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;
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.
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
