[rds-devel] potential connection stall with congestion?

[Pradeep]

On 8/4/2008 11:04 PM, Andy Grover wrote:
> Pradeep wrote:
>> It appears that with lot of congestion updates, there is chance
>> that the regular messages may not get acked. Suppose
>> rds_ib_recv_cq_comp_handler() processes multiple messages and
>> the last one happens to be a congestion map message. In this
>> case, the state.ack_next will be set to zero (congestion map
>> message is sent with sequence number zero). Since this is
>> lower than the current i_ack_next, rds_ib_set_ack() won't
>> update i_ack_next. On send side, since ack hasn't arrived
>> the rs_snd_bytes will keep on increasing and rds_sendmsg()
>> will start returning error(EAGAIN).
>>
>> I think the following change will fix it. However I'm not
>> sure it will break something else..
>>
>> rds_ib_process_recv():
>>
>> if(be64_to_cpu(hdr->h_sequence) > state->ack_next) {
>>         state->ack_next = be64_to_cpu(hdr->h_sequence);
>>         state->ack_next_valid = 1;
>> }
>
> OK so congestion updates don't count towards generating acks. So let's 
> just set the fields in state struct for non-cong data only:
>
> if (ibinc->ii_inc.i_hdr.h_flags == RDS_FLAG_CONG_BITMAP)
>     rds_ib_cong_recv(conn, ibinc);
> else {
>     rds_recv_incoming(conn, conn->c_faddr, conn->c_laddr,
>                    &ibinc->ii_inc, GFP_ATOMIC,
>                    KM_SOFTIRQ0);
>     state->ack_next = be64_to_cpu(hdr->h_sequence);
>     state->ack_next_valid = 1;
> }
>
> Thoughts?

Yes, this makes sense.

-Pradeep
>
>> There is also a problem in the KERNEL_HAS_ATOMIC64 version
>> of rds_ib_set_ack(). Shouldn't this set i_ack_next in the
>> same way as the other case?
>
> Yeah, makes sense.
>
> Regards -- Andy