: Removal of spinlocks/rw_locks in ccid3.c / packet_history.c:

With regard to the previous patch 9/9 I wonder if it is possible to remove the Reader/Writer spinlock used in the test tree in packet_history.c (field member @lock in struct tfrc_rx_hist).

If the socket is protected by the socket lock, and the same backlog handler does not run concurrently, then the RX history rw_lock is redundant. I haven't changed anything, but am about to reformat the patches so that each of them compiles standalone, it is a good time to address this.

The point that I find confusing here is that several new cards (e.g. e1000) can have multiple TX/RX queues on the same card: I wonder whether that will make a difference to the DCCP sk backlog handler.

Comments?

Em Sat, Nov 03, 2007 at 01:50:20PM +0000, Gerrit Renker escreveu:

I think that you are right, we can't process two packets at the same time for the same socket, more locks than the socket lock are not needed at all.

With regards to multiple tx/rx queues: same reasoning applies, multiple packets are being received/sent, but per socket serialization happens at l4.

- Arnaldo

Thank you for the clarification - it is good when this code goes (since it propagated to ccid4,too); I will take them out. I would like to re-submit a bisectable and repackaged set of the old CCID3 patches, comments and suggestions are welcome (aiming at fewer patches, since most of the patches just navigate around and are at micro-granule level).

Gerrit

Following the earlier RFC and Arnaldo's comments, the unnecessary spinlocks and reader/writer locks used so far for the TFRC RX and TX history structures have been removed.

It has taken some time since it involved testing as well; I think it is correct now. Below is the changelog of the updated tree. I have not yet put it online, first due to testing, and I think that the CCID4 subtree also needs an update; to be done.

The patch set has so far not been combined into larger units, but this will be done when this change is through.

The changelog below records the changes made to individual patches. The `interdiff' between the old test tree (online) and the new test tree (not yet online) is at the end of this message: it is small.

TX History Changes
------------------
[TFRC]: Migrate TX history to singly-linked list
- removed global declaration of reader/writer lock in packet_history.c (DEFINE_RWLOCK)

[TFRC]: History allocation / deallocation routines
- removed the locks in tx_hist_add() and tx_hist_cleanup()
- revised the algorithm which "cuts off" the tail of the singly-linked list
- this lead to much shorter functions

[TFRC]: Add history query/lookup function

RX History Changes
------------------
[TFRC]: New receiver history data structure
- removed the @lock field member of struct tfrc_rx_hist

[TFRC]: New RX History Step 2 - Initialisation and cleanup
- removed lock initialisation from tfrc_rx_hist_init()

[CCID 3]: New RX History Step 4/5 - Integrate Receiver RTT sampling (part 3 of 3)
- removed the use of locks in ccid3_hc_rx_packet_recv()

All other patches not mentioned have been ajdusted (but not changed in content) to match the above update.

          -----------------------------------------------------
          Interdiff from previous test tree ..... test tree now
          -----------------------------------------------------

@@ -112,14 +112,12 @@ struct tfrc_rx_hist_entry {
  *   @loss_count:	Number of entries in circular history
  *   @loss_start:	Movable index (for loss detection)
  *   @rtt_sample_prev:  Used during RTT sampling, points to candidate entry
- *   @lock:		Serialize concurrent access
  */
 struct tfrc_rx_hist {
 	struct tfrc_rx_hist_entry	*ring[NDUPACK + 1];
 	u8				loss_count:2,
 					loss_start:2;
 #define rtt_sample_prev			loss_start
-	spinlock_t			lock;
 };
 
 /*

@@ -782,8 +782,6 @@ static void ccid3_hc_rx_packet_recv(struct sock *sk, struct sk_buff *skb)
 	u32 sample, payload_size = skb->len - dccp_hdr(skb)->dccph_doff * 4;
 	u8  is_data_packet = dccp_data_packet(skb);
 
-	spin_lock(&hcrx->ccid3hcrx_hist.lock);
-
 	if (unlikely(hcrx->ccid3hcrx_state = TFRC_RSTATE_NO_DATA)) {
 		if (is_data_packet) {
 			do_feedback = FBACK_INITIAL;

@@ -847,8 +845,6 @@ update_records:
 	tfrc_rx_hist_update(&hcrx->ccid3hcrx_hist, skb, ndp);
 
 done_receiving:
-	spin_unlock(&hcrx->ccid3hcrx_hist.lock);
-
 	if (do_feedback)
 		ccid3_hc_rx_send_feedback(sk, skb, do_feedback);
 }