Re: [PATCH] dropmon: add ability to detect when hardwaredropsrxpackets

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

On Thu, May 14, 2009 at 02:44:08PM +0200, Jiri Pirko wrote:
> Thu, May 14, 2009 at 02:33:00PM CEST, nhorman@tuxdriver.com wrote:
> >> 
> >> The purpose of list_add_rcu() and list_del_rcu() is to be able to permit
> >> concurrent readers to traverse the list while you are modifying that same
> >> list.  You still need something to handle concurrent updates to the list.
> >> The usual approach is to use locks, in this case, to hold trace_state_lock
> >> across the addition, given that it is already held across the removal.
> >> 
> >> The reason that I expect holding the lock to be OK is that you cannot
> >> add elements faster than you delete them long-term, so given that you
> >> are willing to hold the lock over deletions, you are probably OK also
> >> holding that same lock over additions.  But please let me know if that
> >> is not the case!
> >> 
> >> 							Thanx, Paul
> >> 
> >
> >
> >Ahh, I did learn something then :).  That makes sense, ok. New patch attached.
> >
> >Patch to add the ability to detect drops in hardware interfaces via dropwatch.
> >Adds a tracepoint to net_rx_action to signal everytime a napi instance is
> >polled.  The dropmon code then periodically checks to see if the rx_frames
> >counter has changed, and if so, adds a drop notification to the netlink
> >protocol, using the reserved all-0's vector to indicate the drop location was in
> >hardware, rather than somewhere in the code.
> >   
> >change notes:
> >1) added use of trace_state_lock around list_add_rcu operation
> >
> >
> >Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> >
> >
> > include/linux/net_dropmon.h |    7 ++
> > include/trace/napi.h        |   11 ++++
> > net/core/dev.c              |    5 +
> > net/core/drop_monitor.c     |  115 ++++++++++++++++++++++++++++++++++++++++++--
> > net/core/net-traces.c       |    4 +
> > net/core/netpoll.c          |    2 
> > 6 files changed, 139 insertions(+), 5 deletions(-)
> >
> >diff --git a/include/linux/net_dropmon.h b/include/linux/net_dropmon.h

[ . . . ]

> > static int set_all_monitor_traces(int state)
> > {
> > 	int rc = 0;
> >+	struct dm_hw_stat_delta *new_stat = NULL;
> >+
> >+	spin_lock(&trace_state_lock);
> > 
> > 	switch (state) {
> > 	case TRACE_ON:
> > 		rc |= register_trace_kfree_skb(trace_kfree_skb_hit);
> >+		rc |= register_trace_napi_poll(trace_napi_poll_hit);
> > 		break;
> > 	case TRACE_OFF:
> > 		rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit);
> >+		rc |= unregister_trace_napi_poll(trace_napi_poll_hit);
> > 
> > 		tracepoint_synchronize_unregister();
> >+
> >+		/*
> >+		 * Clean the device list
> >+		 */
> >+		list_for_each_entry_rcu(new_stat, &hw_stats_list, list) {
> 		^^^^^^^^^^^^^^^^^^^^^^^
> This is meaningless here. Use list_for_each_entry_rcu only under rcu_read_lock.
> Also it would be good to use list_for_each_entry_safe here since you're
> modifying the list.

Either way works, as the list_del_rcu() leaves the forward pointers
intact.  So I don't have an opinion either way on this particular piece
of code.  More experience will be needed to work out which approach is
less confusing.  :-/

If this code was shared between the read side and the update side, then
you really would want to be able to use list_for_each_entry_rcu() on the
update side.

							Thanx, Paul