| Message ID | 66c7c361d03a72de6a216fd1d6ffa0bc@imap.linux.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
> +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
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
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
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 --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; }