* [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity [not found] <20180725182019.31518-4-stephen@networkplumber.org> @ 2018-10-26 14:55 ` Anatoly Burakov 2018-10-26 20:41 ` Thomas Monjalon 2018-11-13 18:03 ` [dpdk-stable] [PATCH v2] " Anatoly Burakov 0 siblings, 2 replies; 5+ messages in thread From: Anatoly Burakov @ 2018-10-26 14:55 UTC (permalink / raw) To: dev; +Cc: thomas, stable, stephen, Stephen Hemminger EAL should not crash when setting alarm fails. Also, remove the profanity in error message. Fixes: daf9bfca717e ("ipc: remove thread for async requests") Cc: stable@dpdk.org Cc: stephen@networkplumber.org Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> --- lib/librte_eal/common/eal_common_proc.c | 28 ++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c index 97663d3ba..d17131296 100644 --- a/lib/librte_eal/common/eal_common_proc.c +++ b/lib/librte_eal/common/eal_common_proc.c @@ -827,6 +827,27 @@ mp_request_async(const char *dst, struct rte_mp_msg *req, goto fail; } + /* + * set the alarm before sending message. there are two possible error + * scenarios to consider here: + * + * - if the alarm set fails, we free the memory right there + * - if the alarm set succeeds but sending message fails, then the alarm + * will trigger and clean up the memory + * + * Even if the alarm triggers too early (i.e. immediately), we're still + * holding the lock to pending requests queue, so the interrupt thread + * will just spin until we release the lock, and either release the + * memory, or doesn't find any pending requests in the queue because we + * never added any due to send message failure. + */ + if (rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000, + async_reply_handle, pending_req) < 0) { + RTE_LOG(ERR, EAL, "Fail to set alarm for request %s:%s\n", + dst, req->name); + goto fail; + } + ret = send_msg(dst, req, MP_REQ); if (ret < 0) { RTE_LOG(ERR, EAL, "Fail to send request %s:%s\n", @@ -841,13 +862,6 @@ mp_request_async(const char *dst, struct rte_mp_msg *req, param->user_reply.nb_sent++; - if (rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000, - async_reply_handle, pending_req) < 0) { - RTE_LOG(ERR, EAL, "Fail to set alarm for request %s:%s\n", - dst, req->name); - rte_panic("Fix the above shit to properly free all memory\n"); - } - return 0; fail: free(pending_req); -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity 2018-10-26 14:55 ` [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity Anatoly Burakov @ 2018-10-26 20:41 ` Thomas Monjalon 2018-10-30 10:31 ` Burakov, Anatoly 2018-11-13 18:03 ` [dpdk-stable] [PATCH v2] " Anatoly Burakov 1 sibling, 1 reply; 5+ messages in thread From: Thomas Monjalon @ 2018-10-26 20:41 UTC (permalink / raw) To: Anatoly Burakov; +Cc: stable, dev, stephen, Stephen Hemminger 26/10/2018 16:55, Anatoly Burakov: > --- a/lib/librte_eal/common/eal_common_proc.c > +++ b/lib/librte_eal/common/eal_common_proc.c > + /* > + * set the alarm before sending message. there are two possible error > + * scenarios to consider here: > + * > + * - if the alarm set fails, we free the memory right there > + * - if the alarm set succeeds but sending message fails, then the alarm > + * will trigger and clean up the memory > + * > + * Even if the alarm triggers too early (i.e. immediately), we're still > + * holding the lock to pending requests queue, so the interrupt thread > + * will just spin until we release the lock, and either release the > + * memory, or doesn't find any pending requests in the queue because we > + * never added any due to send message failure. > + */ > + if (rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000, > + async_reply_handle, pending_req) < 0) { > + RTE_LOG(ERR, EAL, "Fail to set alarm for request %s:%s\n", > + dst, req->name); > + goto fail; > + } ret variable is not set and not initialized. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity 2018-10-26 20:41 ` Thomas Monjalon @ 2018-10-30 10:31 ` Burakov, Anatoly 0 siblings, 0 replies; 5+ messages in thread From: Burakov, Anatoly @ 2018-10-30 10:31 UTC (permalink / raw) To: Thomas Monjalon; +Cc: stable, dev, stephen, Stephen Hemminger On 26-Oct-18 9:41 PM, Thomas Monjalon wrote: > 26/10/2018 16:55, Anatoly Burakov: >> --- a/lib/librte_eal/common/eal_common_proc.c >> +++ b/lib/librte_eal/common/eal_common_proc.c >> + /* >> + * set the alarm before sending message. there are two possible error >> + * scenarios to consider here: >> + * >> + * - if the alarm set fails, we free the memory right there >> + * - if the alarm set succeeds but sending message fails, then the alarm >> + * will trigger and clean up the memory >> + * >> + * Even if the alarm triggers too early (i.e. immediately), we're still >> + * holding the lock to pending requests queue, so the interrupt thread >> + * will just spin until we release the lock, and either release the >> + * memory, or doesn't find any pending requests in the queue because we >> + * never added any due to send message failure. >> + */ >> + if (rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000, >> + async_reply_handle, pending_req) < 0) { >> + RTE_LOG(ERR, EAL, "Fail to set alarm for request %s:%s\n", >> + dst, req->name); >> + goto fail; >> + } > > ret variable is not set and not initialized. > Oh, right. Apologies. Will send a v2. -- Thanks, Anatoly ^ permalink raw reply [flat|nested] 5+ messages in thread
* [dpdk-stable] [PATCH v2] eal/mp: remove rte_panic and profanity 2018-10-26 14:55 ` [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity Anatoly Burakov 2018-10-26 20:41 ` Thomas Monjalon @ 2018-11-13 18:03 ` Anatoly Burakov 2018-11-13 23:03 ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon 1 sibling, 1 reply; 5+ messages in thread From: Anatoly Burakov @ 2018-11-13 18:03 UTC (permalink / raw) To: dev; +Cc: stable, stephen, Stephen Hemminger EAL should not crash when setting alarm fails. Also, remove the profanity in error message. Fixes: daf9bfca717e ("ipc: remove thread for async requests") Cc: stable@dpdk.org Cc: stephen@networkplumber.org Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> --- Notes: v2: fix uninitialized variable lib/librte_eal/common/eal_common_proc.c | 31 ++++++++++++++++++------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c index 97663d3ba..f65ef56c0 100644 --- a/lib/librte_eal/common/eal_common_proc.c +++ b/lib/librte_eal/common/eal_common_proc.c @@ -800,7 +800,7 @@ mp_request_async(const char *dst, struct rte_mp_msg *req, { struct rte_mp_msg *reply_msg; struct pending_request *pending_req, *exist; - int ret; + int ret = -1; pending_req = calloc(1, sizeof(*pending_req)); reply_msg = calloc(1, sizeof(*reply_msg)); @@ -827,6 +827,28 @@ mp_request_async(const char *dst, struct rte_mp_msg *req, goto fail; } + /* + * set the alarm before sending message. there are two possible error + * scenarios to consider here: + * + * - if the alarm set fails, we free the memory right there + * - if the alarm set succeeds but sending message fails, then the alarm + * will trigger and clean up the memory + * + * Even if the alarm triggers too early (i.e. immediately), we're still + * holding the lock to pending requests queue, so the interrupt thread + * will just spin until we release the lock, and either release the + * memory, or doesn't find any pending requests in the queue because we + * never added any due to send message failure. + */ + if (rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000, + async_reply_handle, pending_req) < 0) { + RTE_LOG(ERR, EAL, "Fail to set alarm for request %s:%s\n", + dst, req->name); + ret = -1; + goto fail; + } + ret = send_msg(dst, req, MP_REQ); if (ret < 0) { RTE_LOG(ERR, EAL, "Fail to send request %s:%s\n", @@ -841,13 +863,6 @@ mp_request_async(const char *dst, struct rte_mp_msg *req, param->user_reply.nb_sent++; - if (rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000, - async_reply_handle, pending_req) < 0) { - RTE_LOG(ERR, EAL, "Fail to set alarm for request %s:%s\n", - dst, req->name); - rte_panic("Fix the above shit to properly free all memory\n"); - } - return 0; fail: free(pending_req); -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] eal/mp: remove rte_panic and profanity 2018-11-13 18:03 ` [dpdk-stable] [PATCH v2] " Anatoly Burakov @ 2018-11-13 23:03 ` Thomas Monjalon 0 siblings, 0 replies; 5+ messages in thread From: Thomas Monjalon @ 2018-11-13 23:03 UTC (permalink / raw) To: Anatoly Burakov; +Cc: dev, stable, stephen, Stephen Hemminger 13/11/2018 19:03, Anatoly Burakov: > EAL should not crash when setting alarm fails. Also, remove the > profanity in error message. > > Fixes: daf9bfca717e ("ipc: remove thread for async requests") > > Cc: stable@dpdk.org > Cc: stephen@networkplumber.org > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> Applied, thanks ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-13 23:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20180725182019.31518-4-stephen@networkplumber.org>
2018-10-26 14:55 ` [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity Anatoly Burakov
2018-10-26 20:41 ` Thomas Monjalon
2018-10-30 10:31 ` Burakov, Anatoly
2018-11-13 18:03 ` [dpdk-stable] [PATCH v2] " Anatoly Burakov
2018-11-13 23:03 ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).