diff mbox

[v2,2/4] rsockets: retry for completion events upon interruption

Message ID 66c7c361d03a72de6a216fd1d6ffa0bc@imap.linux.ibm.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Sreedhar Kodali Sept. 5, 2014, 1:14 p.m. UTC
From: Sreedhar Kodali <srkodali@linux.vnet.ibm.com>

     While waiting for a completion queue event, rsocket's logic by 
default
     bails out when interrupted.  Because of this, on the passive side
     ongoing connection establishments are abruptly terminated without
     fully accepting the incoming connection requests.

     The solution is to modify the completion event waiting logic to 
ensure
     that it retries for the event upon interruption instead of returning
     with an error.  This behavior is gated by a runtime parameter
     'restart_onintr' with the associated configuration file so it does
     not affect in any way the rest of rsocket applications.

     Signed-off-by: Sreedhar Kodali <srkodali@linux.vnet.ibm.com>
     Reviewed-by: Pradeep Satyanarayana <pradeeps@linux.vnet.ibm.com>
     ---


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Hefty, Sean Sept. 17, 2014, 4:27 p.m. UTC | #1
> +resume_get_cq_event:
>   	ret = ibv_get_cq_event(rs->cm_id->recv_cq_channel, &cq, &context);
>   	if (!ret) {
>   		ibv_ack_cq_events(rs->cm_id->recv_cq, 1);
>   		rs->cq_armed = 0;
> +	} else if (restart_onintr == 1 && errno == EINTR) {
> +		errno = 0;
> +		goto resume_get_cq_event;

I'm not convinced that this is desirable behavior.  If the thread waiting for an event was interrupted, why does it make sense to ignore the interrupt and return to waiting?  Couldn't the app detect the return code and call back into rsockets in this case?

If this is desired behavior, why add a config option?

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Sept. 17, 2014, 10:13 p.m. UTC | #2
On Wed, Sep 17, 2014 at 04:27:05PM +0000, Hefty, Sean wrote:
> > +resume_get_cq_event:
> >   	ret = ibv_get_cq_event(rs->cm_id->recv_cq_channel, &cq, &context);
> >   	if (!ret) {
> >   		ibv_ack_cq_events(rs->cm_id->recv_cq, 1);
> >   		rs->cq_armed = 0;
> > +	} else if (restart_onintr == 1 && errno == EINTR) {
> > +		errno = 0;
> > +		goto resume_get_cq_event;
> 
> I'm not convinced that this is desirable behavior.  If the thread
> waiting for an event was interrupted, why does it make sense to
> ignore the interrupt and return to waiting?  Couldn't the app detect
> the return code and call back into rsockets in this case?

More broadly, it doesn't make sense for a library to check EINTR in
one place without also checking every blocking syscall it makes.

Realistically, apps that want to use signals in this way will need to
see the EINTR at the app level, while apps that don't care should be
using SA_RESTART to prevent EINTR from being generated.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sreedhar Kodali Sept. 18, 2014, 11 a.m. UTC | #3
On 2014-09-17 21:57, Hefty, Sean wrote:
>> +resume_get_cq_event:
>>   	ret = ibv_get_cq_event(rs->cm_id->recv_cq_channel, &cq, &context);
>>   	if (!ret) {
>>   		ibv_ack_cq_events(rs->cm_id->recv_cq, 1);
>>   		rs->cq_armed = 0;
>> +	} else if (restart_onintr == 1 && errno == EINTR) {
>> +		errno = 0;
>> +		goto resume_get_cq_event;
> 
> I'm not convinced that this is desirable behavior.  If the thread
> waiting for an event was interrupted, why does it make sense to ignore
> the interrupt and return to waiting?  Couldn't the app detect the
> return code and call back into rsockets in this case?
> 
> If this is desired behavior, why add a config option?
> 
> - Sean

Hi Sean,

I have reworked the patch by dropping config option.  Also, we no
longer resume event listening on interruption.  Instead, we desist
from changing state to error upon interruption so the caller can
choose appropriate action.  Please see v5 of the patch.

Thank You.

- Sreedhar

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sreedhar Kodali Sept. 18, 2014, 11:03 a.m. UTC | #4
On 2014-09-18 03:43, Jason Gunthorpe wrote:
> On Wed, Sep 17, 2014 at 04:27:05PM +0000, Hefty, Sean wrote:
>> > +resume_get_cq_event:
>> >   	ret = ibv_get_cq_event(rs->cm_id->recv_cq_channel, &cq, &context);
>> >   	if (!ret) {
>> >   		ibv_ack_cq_events(rs->cm_id->recv_cq, 1);
>> >   		rs->cq_armed = 0;
>> > +	} else if (restart_onintr == 1 && errno == EINTR) {
>> > +		errno = 0;
>> > +		goto resume_get_cq_event;
>> 
>> I'm not convinced that this is desirable behavior.  If the thread
>> waiting for an event was interrupted, why does it make sense to
>> ignore the interrupt and return to waiting?  Couldn't the app detect
>> the return code and call back into rsockets in this case?
> 
> More broadly, it doesn't make sense for a library to check EINTR in
> one place without also checking every blocking syscall it makes.
> 
> Realistically, apps that want to use signals in this way will need to
> see the EINTR at the app level, while apps that don't care should be
> using SA_RESTART to prevent EINTR from being generated.
> 
> Jason

Hi Jason,

Thanks for your review comments.  I have reworked on the patch based
on your suggestions.  Please look into v5 of the patch that is sent
separately.

Thank You.

- Sreedhar

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/src/rsocket.c b/src/rsocket.c
index 78261dc..b70d56a 100644
--- a/src/rsocket.c
+++ b/src/rsocket.c
@@ -115,6 +115,7 @@  static uint16_t def_rqsize = 384;
  static uint32_t def_mem = (1 << 17);
  static uint32_t def_wmem = (1 << 17);
  static uint32_t polling_time = 10;
+static uint16_t restart_onintr = 0;

  /*
   * Immediate data format is determined by the upper bits
@@ -542,6 +543,11 @@  void rs_configure(void)
  		def_iomap_size = (uint8_t) rs_value_to_scale(
  			(uint16_t) rs_scale_to_value(def_iomap_size, 8), 8);
  	}
+
+	if ((f = fopen(RS_CONF_DIR "/restart_onintr", "r"))) {
+		(void) fscanf(f, "%hu", &restart_onintr);
+		fclose(f);
+	}
  	init = 1;
  out:
  	pthread_mutex_unlock(&mut);
@@ -1969,10 +1975,14 @@  static int rs_get_cq_event(struct rsocket *rs)
  	if (!rs->cq_armed)
  		return 0;

+resume_get_cq_event:
  	ret = ibv_get_cq_event(rs->cm_id->recv_cq_channel, &cq, &context);
  	if (!ret) {
  		ibv_ack_cq_events(rs->cm_id->recv_cq, 1);
  		rs->cq_armed = 0;
+	} else if (restart_onintr == 1 && errno == EINTR) {
+		errno = 0;
+		goto resume_get_cq_event;
  	} else if (errno != EAGAIN) {
  		rs->state = rs_error;
  	}