| But why do you have to? If it is a REQUEST, don't bump the GSS... wait, | I actually took the time and went to re-read the RFC and I think the | changelog Wei wrote is misleading, as: | | -------------------- 8< ------------------------------------------------ | A client in the REQUEST state SHOULD use an exponential-backoff timer | to send new DCCP-Request packets if no response is received. The | first retransmission should occur after approximately one second, | backing off to not less than one packet every 64 seconds; or the | endpoint can use whatever retransmission strategy is followed for | retransmitting TCP SYNs. Each new DCCP-Request MUST increment the | Sequence Number by one and MUST contain the same Service Code and | application data as the original DCCP-Request. | -------------------- 8< ------------------------------------------------ | | So the fix would be for not bumping GSS on the first packet? I guess so, | the flow is: | | To create a socket: | | dccp_v4_connect(): sets iss | dccp_connect() | dccp_connect_init(): iss = gss | dccp_transmit_skb: gss += 1 | | So the problem is that if this is the first packet being sent on a | connection we should not do the gss += 1... | Yes exactly that is the problem. And I find it ugly, too, to first set ISS, and then set GSS=ISS+1. | > Here is the sketch of the solution I was working on, I'd be glad for input: | > * dccp_entail(), skb_clone(), dccp_transmit_skb() and setting of the icsk | > retransmit timer always occur together; | > * as soon as skb_clone() is called, skb->cloned is true; | > * so the idea is to | > - use dccp_transmit_skb() to transmit the un-cloned skb; | > - sk->send_head = skb_clone(skb, gfp_any()) | > - need some more careful analysis, but I think the set_owner_w() can | > also be removed, needs checking against the WARN_ON in | > dccp_transmit_skb(). | > | > Or maybe there is an easier way - the objective is to have a clear | > indicator whether the skb is a retransmitted one or the first one | > (since a similar problem arises when sending Close(Req) packets). | | ...since I think I got to the same page as you guys... :-) Lets continue | from this point: | | I think we have to use set_owner_w, its safer, we better get the | connection stuck due to using all its socket send buffer space if the | packet is never freed because of some stack bug than to get the whole | machine with a problem, even if after a long time, if we leak these | packets. | | Perhaps we can use a flag in skb->cb that we would set only on the first | packet? Nah, I think we should have a __dccp_transmit_skb, that... code, | lets show it as a patch, see at the end of this message. | I had a further look, it is actually possible. Your point about set_owner_w is good - I've had to reboot three times today because of non-freed references. What I found so far is that skb_cloned(skb) in dccp_retransmit_skb is never true, since we put the original skb onto the sk->send_head, so dccp_retransmit_skb() always calls skb_clone. That is why dccp_transmit_skb() never complains that skb->sk is already set. What I was hoping for was a rewrite of the dccp_retransmit_skb(), so that the original skb is transmitted first and the clone put onto the send-head. But doesn't work since queue_xmit frees the original skb, so was stuck there. | WRT Close(Req) I think it is right as it is, i.e. we should bump GSS, if | not we'll send the packet with the same sequence number as the last | packet sent, i.e. the problem is just on the very first packet. | | Or am I missing something? Correct - this applies only to the initial packet, so we can safely increment GSS on entering dccp_transmit_skb(). I will look at your code at home, we will be shutting down here everything in 15 minutes. Thanks a lot for the input, Gerrit -- 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
