Hello, below I have corrected where you pointed out that I was wrong. However, I think we are both barking up the wrong tree here, i.e. I'd say that neither your or my patch is `better', but rather: the evil lies somewhere else. So no matter which patch lands in Arnaldos tree, the main problem will be unresolved irrespectively. Both patches, when we put aside other quibbles here, are in effect a variation of the same theme. | > 1) The race condition is not tackled. As before, you allow several routines running | > asynchronously in software interrupts (timer interrupt, net backlog softirq) and | > in user context (dccp_sendmsg) to write/read on t_nom. Without locking, you will | > have the race condition that one routine performs a write (e.g. sub_usecs) while | > another performs a read and sends the packet to early. | | I don't believe so. For a start now I'm only altering t_nom in one | place except for the initial setup which I'll discuss later. And in | addition I'm only doing additions apart from initial setup. As such I | believe mine is just as safe or more so. Went through the patch again and have to agree: there is only the subtraction in rx_packet_recv, which would be debatable. So your patch also, by and large, tries to avoid the race condition by monopolising the access to t_nom. Sorry I didn't realize earlier. | > I am not in support of this patch. This patch is against _working_ code. Instead of solving | > problems, it introduces new ones. | > | > Can I suggest that we spent our time more profitably on fixing the many bugs that exist? | > Fix broken, not working code. | > | I am fixing broken code. I don't think either of our initial solutions | were correct. I agree but my point is that we are wasting our energy on a discussion of patches which, by and large, effect the same: before you sent this email, I had consulted with colleagues about this and will send the notes in a follow-up email. | > | hctx->ccid3hctx_t_nom = now; | > | + timeval_add_usecs(&hctx->ccid3hctx_t_nom, USEC_PER_SEC); | > BUG: The first packet will be delayed for 1 second instead of being sent immediately as specified | > in RFC 3448, 4.6. | > | No you are reading my code incorrectly. It breaks out of the switch | and sends immediately as returns 0. You are right and I had meant to point this out in a follow-up: the code above has the same effect as before, there is no delay. Sorry. | > | @@ -486,6 +474,7 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb) | > | hctx->ccid3hctx_x = scaled_div(w_init << 6, r_sample); | > | hctx->ccid3hctx_t_ld = now; | > | | > | + timeval_sub_usecs(&hctx->ccid3hctx_t_nom, USEC_PER_SEC); | > | ccid3_update_send_time(hctx); | > Why this complicated code - it was simpler before. Now you first add 1 second, then send the packet | > immediately and then you subtract again. | > | Overall I think this is much simpler. Before we were adding and | subtracting with each change but now we are doing only on the first | feedback (not immediately as you say) The race-condition patch also removes the add/subtract pair. Please let me restate the above - I think it is a matter of programming taste - your patch does affect some simplification, mine also does; but I'd say neither is `better'. | | > | @@ -539,11 +528,7 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb) | > | - /* | > | - * Schedule no feedback timer to expire in | > | - * max(t_RTO, 2 * s/X) = max(t_RTO, 2 * t_ipi) | > | - */ | > | - t_nfb = max(hctx->ccid3hctx_t_rto, 2 * hctx->ccid3hctx_t_ipi); | > | + t_nfb = hctx->ccid3hctx_t_rto; | > | | > BUG: This removes conformance with RFC 3448, section 4.3 and section 4.4. | > | I think I see what you are saying but the code is/was wrong here. | Section 4.3 and 4.4 talks about t_mbi, not t_ipi. t_mbi is a constant | of 64 seconds. I meant the passages where the nofeedback timer is set to expire after max(4*R, 2*s/X), where t_ipi = s/X. Now it is only reset to max(4*R). However - again this is a sideline and not central to what I think your patch tries to achieve. | can you move the refactor patch one up in the sequence so I have shifted it up by 4 - from 3f to 3a - and refreshed the offsets of the other patches. Summary of differences ---------------------- I would like to come to a kind of settlement for this discussion. We are both arguing that each of our patches is `better', but I think this discussion leads nowhere, since the real problems (see follow-up) remain untouched. Taking the subtraction in packet_recv aside, I think it the patch I submitted the exclusive access to t_nom is clearer to see. In your solution, the access is within a function (ccid3_update_send_time), which could end up being called by other functions and thus reintroduce the race condition through the back door. It is safe only because this function has only a single caller at the moment. In the patch I submitted (3d/3e) t_nom is only updated within send_packet and not via a function which is available to other ccid3 routines. Recomputing t_ipi and delta on each send is slightly more expensive. - To unsubscribe from this list: send the line "unsubscribe dccp" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
