DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/4] small cleanups
@ 2018-07-25 18:20 Stephen Hemminger
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 1/4] arm: remove profanity in comment Stephen Hemminger
                   ` (3 more replies)
  0 siblings, 4 replies; 24+ messages in thread
From: Stephen Hemminger @ 2018-07-25 18:20 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger

These were discovered when running internal policy checker
to try and get rid of potentially offensive language in DPDK.
After looking at a couple of these places, they were also
places where some improvement could be made.

Stephen Hemminger (4):
  arm: remove profanity in comment
  bnx2x: remove profanity
  eal: don't crash if alarm set fails
  ixgbe: remove mild profanity

 drivers/net/bnx2x/elink.c                              |  4 ++--
 drivers/net/ixgbe/ixgbe_rxtx.c                         |  3 +--
 lib/librte_eal/common/eal_common_proc.c                | 10 ++++------
 lib/librte_eal/common/include/arch/arm/rte_cycles_32.h |  4 ++--
 4 files changed, 9 insertions(+), 12 deletions(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [dpdk-dev] [PATCH 1/4] arm: remove profanity in comment
  2018-07-25 18:20 [dpdk-dev] [PATCH 0/4] small cleanups Stephen Hemminger
@ 2018-07-25 18:20 ` Stephen Hemminger
  2018-10-24 23:50   ` Thomas Monjalon
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity Stephen Hemminger
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 24+ messages in thread
From: Stephen Hemminger @ 2018-07-25 18:20 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Stephen Hemminger

Update comment to describe the problem better without
risk of being offensive.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/librte_eal/common/include/arch/arm/rte_cycles_32.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/include/arch/arm/rte_cycles_32.h b/lib/librte_eal/common/include/arch/arm/rte_cycles_32.h
index c4f974feb3da..859b09748c56 100644
--- a/lib/librte_eal/common/include/arch/arm/rte_cycles_32.h
+++ b/lib/librte_eal/common/include/arch/arm/rte_cycles_32.h
@@ -29,8 +29,8 @@ extern "C" {
 #ifndef RTE_ARM_EAL_RDTSC_USE_PMU
 
 /**
- * This call is easily portable to any ARM architecture, however,
- * it may be damn slow and inprecise for some tasks.
+ * This call is easily portable to any architecture, however,
+ * it may require a system call and inprecise for some tasks.
  */
 static inline uint64_t
 __rte_rdtsc_syscall(void)
-- 
2.17.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity
  2018-07-25 18:20 [dpdk-dev] [PATCH 0/4] small cleanups Stephen Hemminger
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 1/4] arm: remove profanity in comment Stephen Hemminger
@ 2018-07-25 18:20 ` Stephen Hemminger
  2018-09-18  9:40   ` Thomas Monjalon
  2018-10-26 14:56   ` Burakov, Anatoly
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails Stephen Hemminger
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 4/4] ixgbe: remove mild profanity Stephen Hemminger
  3 siblings, 2 replies; 24+ messages in thread
From: Stephen Hemminger @ 2018-07-25 18:20 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Stephen Hemminger

No need for profanity in comments.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/bnx2x/elink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bnx2x/elink.c b/drivers/net/bnx2x/elink.c
index 34a29373af3b..08fe817720a1 100644
--- a/drivers/net/bnx2x/elink.c
+++ b/drivers/net/bnx2x/elink.c
@@ -3993,11 +3993,11 @@ static elink_status_t elink_get_mod_abs_int_cfg(struct bnx2x_softc *sc,
 			   PORT_HW_CFG_E3_MOD_ABS_MASK) >>
 		    PORT_HW_CFG_E3_MOD_ABS_SHIFT;
 
-		/* Should not happen. This function called upon interrupt
+		/*
+		 * Should not happen. This function called upon interrupt
 		 * triggered by GPIO ( since EPIO can only generate interrupts
 		 * to MCP).
 		 * So if this function was called and none of the GPIOs was set,
-		 * it means the shit hit the fan.
 		 */
 		if ((cfg_pin < PIN_CFG_GPIO0_P0) ||
 		    (cfg_pin > PIN_CFG_GPIO3_P1)) {
-- 
2.17.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails
  2018-07-25 18:20 [dpdk-dev] [PATCH 0/4] small cleanups Stephen Hemminger
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 1/4] arm: remove profanity in comment Stephen Hemminger
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity Stephen Hemminger
@ 2018-07-25 18:20 ` Stephen Hemminger
  2018-07-26  9:34   ` Burakov, Anatoly
                     ` (2 more replies)
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 4/4] ixgbe: remove mild profanity Stephen Hemminger
  3 siblings, 3 replies; 24+ messages in thread
From: Stephen Hemminger @ 2018-07-25 18:20 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Stephen Hemminger

There is no need to call rte_exit and crash the application here;
better to let the application handle the error itself.

Remove the gratuitous profanity which would be visible if
the rte_exit was still there.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/librte_eal/common/eal_common_proc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index 9fcb9121908d..07b7579c565a 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -841,14 +841,12 @@ 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) {
+	ret = rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000,
+				async_reply_handle, pending_req);
+	if (ret < 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;
+	return ret;
 fail:
 	free(pending_req);
 	free(reply_msg);
-- 
2.17.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [dpdk-dev] [PATCH 4/4] ixgbe: remove mild profanity
  2018-07-25 18:20 [dpdk-dev] [PATCH 0/4] small cleanups Stephen Hemminger
                   ` (2 preceding siblings ...)
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails Stephen Hemminger
@ 2018-07-25 18:20 ` Stephen Hemminger
  2018-10-16 15:00   ` Ferruh Yigit
  3 siblings, 1 reply; 24+ messages in thread
From: Stephen Hemminger @ 2018-07-25 18:20 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Stephen Hemminger

At the tail end of comment about barriers (I feel your pain);
remove mild profanity.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 354181664202..9e10e15cd34f 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2057,8 +2057,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
 		 * of the ixgbe PMD.
 		 *
 		 * TODO:
-		 *    - Get rid of "volatile" crap and let the compiler do its
-		 *      job.
+		 *    - Get rid of "volatile"  and let the compiler do its job.
 		 *    - Use the proper memory barrier (rte_rmb()) to ensure the
 		 *      memory ordering below.
 		 */
-- 
2.17.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails Stephen Hemminger
@ 2018-07-26  9:34   ` Burakov, Anatoly
  2018-07-26  9:41   ` Burakov, Anatoly
  2018-10-26 14:55   ` [dpdk-dev] [PATCH] eal/mp: remove rte_panic and profanity Anatoly Burakov
  2 siblings, 0 replies; 24+ messages in thread
From: Burakov, Anatoly @ 2018-07-26  9:34 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Stephen Hemminger

On 25-Jul-18 7:20 PM, Stephen Hemminger wrote:
> There is no need to call rte_exit and crash the application here;
> better to let the application handle the error itself.
> 
> Remove the gratuitous profanity which would be visible if
> the rte_exit was still there.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---

Oops, this was a "debug" message i accidentally left in :( My apologies!

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails Stephen Hemminger
  2018-07-26  9:34   ` Burakov, Anatoly
@ 2018-07-26  9:41   ` Burakov, Anatoly
  2018-09-18  9:43     ` Thomas Monjalon
  2018-10-26 14:55   ` [dpdk-dev] [PATCH] eal/mp: remove rte_panic and profanity Anatoly Burakov
  2 siblings, 1 reply; 24+ messages in thread
From: Burakov, Anatoly @ 2018-07-26  9:41 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Stephen Hemminger

On 25-Jul-18 7:20 PM, Stephen Hemminger wrote:
> There is no need to call rte_exit and crash the application here;
> better to let the application handle the error itself.
> 
> Remove the gratuitous profanity which would be visible if
> the rte_exit was still there.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>   lib/librte_eal/common/eal_common_proc.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
> index 9fcb9121908d..07b7579c565a 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -841,14 +841,12 @@ 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) {
> +	ret = rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000,
> +				async_reply_handle, pending_req);
> +	if (ret < 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");

Profanity aside, i think the message was trying to tell me something - 
namely, that if alarm_set fails, we're risking to leak this memory if 
reply from the peer never comes, and we're risking leaving the 
application hanging because the timeout never triggers. I'm not sure if 
leaving this "to the user" is the right choice, because there is no way 
for the user to free IPC-internal memory if it leaks.

So i think the proper way to handle this would've been to set the alarm 
first, then, if it fails, don't sent the message in the first place.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity Stephen Hemminger
@ 2018-09-18  9:40   ` Thomas Monjalon
  2018-09-18 15:07     ` Stephen Hemminger
  2018-10-26 14:56   ` Burakov, Anatoly
  1 sibling, 1 reply; 24+ messages in thread
From: Thomas Monjalon @ 2018-09-18  9:40 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Stephen Hemminger, Harish Patil, Rasesh Mody

25/07/2018 20:20, Stephen Hemminger:
> No need for profanity in comments.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  drivers/net/bnx2x/elink.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/elink.c b/drivers/net/bnx2x/elink.c
> index 34a29373af3b..08fe817720a1 100644
> --- a/drivers/net/bnx2x/elink.c
> +++ b/drivers/net/bnx2x/elink.c
> @@ -3993,11 +3993,11 @@ static elink_status_t elink_get_mod_abs_int_cfg(struct bnx2x_softc *sc,
>  			   PORT_HW_CFG_E3_MOD_ABS_MASK) >>
>  		    PORT_HW_CFG_E3_MOD_ABS_SHIFT;
>  
> -		/* Should not happen. This function called upon interrupt
> +		/*
> +		 * Should not happen. This function called upon interrupt
>  		 * triggered by GPIO ( since EPIO can only generate interrupts
>  		 * to MCP).
>  		 * So if this function was called and none of the GPIOs was set,
> -		 * it means the shit hit the fan.
>  		 */

It makes the comment ends with a comma, like the end is missing.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails
  2018-07-26  9:41   ` Burakov, Anatoly
@ 2018-09-18  9:43     ` Thomas Monjalon
  2018-09-18 10:16       ` Burakov, Anatoly
  0 siblings, 1 reply; 24+ messages in thread
From: Thomas Monjalon @ 2018-09-18  9:43 UTC (permalink / raw)
  To: Burakov, Anatoly; +Cc: dev, Stephen Hemminger, Stephen Hemminger

26/07/2018 11:41, Burakov, Anatoly:
> On 25-Jul-18 7:20 PM, Stephen Hemminger wrote:
> > There is no need to call rte_exit and crash the application here;
> > better to let the application handle the error itself.
> > 
> > Remove the gratuitous profanity which would be visible if
> > the rte_exit was still there.
> > 
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> > --- a/lib/librte_eal/common/eal_common_proc.c
> > +++ b/lib/librte_eal/common/eal_common_proc.c
> > @@ -841,14 +841,12 @@ 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) {
> > +	ret = rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000,
> > +				async_reply_handle, pending_req);
> > +	if (ret < 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");
> 
> Profanity aside, i think the message was trying to tell me something - 
> namely, that if alarm_set fails, we're risking to leak this memory if 
> reply from the peer never comes, and we're risking leaving the 
> application hanging because the timeout never triggers. I'm not sure if 
> leaving this "to the user" is the right choice, because there is no way 
> for the user to free IPC-internal memory if it leaks.
> 
> So i think the proper way to handle this would've been to set the alarm 
> first, then, if it fails, don't sent the message in the first place.

What should be done here? OK to remove rte_panic for now?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails
  2018-09-18  9:43     ` Thomas Monjalon
@ 2018-09-18 10:16       ` Burakov, Anatoly
  2018-10-24 23:51         ` Thomas Monjalon
  0 siblings, 1 reply; 24+ messages in thread
From: Burakov, Anatoly @ 2018-09-18 10:16 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, Stephen Hemminger, Stephen Hemminger

On 18-Sep-18 10:43 AM, Thomas Monjalon wrote:
> 26/07/2018 11:41, Burakov, Anatoly:
>> On 25-Jul-18 7:20 PM, Stephen Hemminger wrote:
>>> There is no need to call rte_exit and crash the application here;
>>> better to let the application handle the error itself.
>>>
>>> Remove the gratuitous profanity which would be visible if
>>> the rte_exit was still there.
>>>
>>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>>> ---
>>> --- a/lib/librte_eal/common/eal_common_proc.c
>>> +++ b/lib/librte_eal/common/eal_common_proc.c
>>> @@ -841,14 +841,12 @@ 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) {
>>> +	ret = rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000,
>>> +				async_reply_handle, pending_req);
>>> +	if (ret < 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");
>>
>> Profanity aside, i think the message was trying to tell me something -
>> namely, that if alarm_set fails, we're risking to leak this memory if
>> reply from the peer never comes, and we're risking leaving the
>> application hanging because the timeout never triggers. I'm not sure if
>> leaving this "to the user" is the right choice, because there is no way
>> for the user to free IPC-internal memory if it leaks.
>>
>> So i think the proper way to handle this would've been to set the alarm
>> first, then, if it fails, don't sent the message in the first place.
> 
> What should be done here? OK to remove rte_panic for now?
> 

As i said, the above fix is wrong because it leaks memory (however 
unlikely it may be).

The alarm set call should be moved to before we do send_msg() call (and 
goto fail; on failure). That way, even if alarm triggers too early (i.e. 
immediately), the requests tailq will still be locked until we complete 
our request sends - so we appropriately free memory on response, on 
timeout or in our failure handler if alarm set has failed.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity
  2018-09-18  9:40   ` Thomas Monjalon
@ 2018-09-18 15:07     ` Stephen Hemminger
  2018-09-19 16:40       ` Mody, Rasesh
  0 siblings, 1 reply; 24+ messages in thread
From: Stephen Hemminger @ 2018-09-18 15:07 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, Stephen Hemminger, Harish Patil, Rasesh Mody

On Tue, 18 Sep 2018 11:40:28 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 25/07/2018 20:20, Stephen Hemminger:
> > No need for profanity in comments.
> > 
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> >  drivers/net/bnx2x/elink.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/bnx2x/elink.c b/drivers/net/bnx2x/elink.c
> > index 34a29373af3b..08fe817720a1 100644
> > --- a/drivers/net/bnx2x/elink.c
> > +++ b/drivers/net/bnx2x/elink.c
> > @@ -3993,11 +3993,11 @@ static elink_status_t elink_get_mod_abs_int_cfg(struct bnx2x_softc *sc,
> >  			   PORT_HW_CFG_E3_MOD_ABS_MASK) >>
> >  		    PORT_HW_CFG_E3_MOD_ABS_SHIFT;
> >  
> > -		/* Should not happen. This function called upon interrupt
> > +		/*
> > +		 * Should not happen. This function called upon interrupt
> >  		 * triggered by GPIO ( since EPIO can only generate interrupts
> >  		 * to MCP).
> >  		 * So if this function was called and none of the GPIOs was set,
> > -		 * it means the shit hit the fan.
> >  		 */  
> 
> It makes the comment ends with a comma, like the end is missing.
> 
> 
> 

Yes, better language would be.
		/* This should not happen since this function is called
		 * from interrupt triggered by GPI ..

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity
  2018-09-18 15:07     ` Stephen Hemminger
@ 2018-09-19 16:40       ` Mody, Rasesh
  2018-10-16 14:45         ` Ferruh Yigit
  0 siblings, 1 reply; 24+ messages in thread
From: Mody, Rasesh @ 2018-09-19 16:40 UTC (permalink / raw)
  To: Stephen Hemminger, Thomas Monjalon; +Cc: dev, Stephen Hemminger, Patil, Harish

>From: Stephen Hemminger <stephen@networkplumber.org>
>Sent: Tuesday, September 18, 2018 8:07 AM
>
>On Tue, 18 Sep 2018 11:40:28 +0200
>Thomas Monjalon <thomas@monjalon.net> wrote:
>
>> 25/07/2018 20:20, Stephen Hemminger:
>> > No need for profanity in comments.
>> >
>> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>> > ---
>> >  drivers/net/bnx2x/elink.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/bnx2x/elink.c b/drivers/net/bnx2x/elink.c
>> > index 34a29373af3b..08fe817720a1 100644
>> > --- a/drivers/net/bnx2x/elink.c
>> > +++ b/drivers/net/bnx2x/elink.c
>> > @@ -3993,11 +3993,11 @@ static elink_status_t
>elink_get_mod_abs_int_cfg(struct bnx2x_softc *sc,
>> >                        PORT_HW_CFG_E3_MOD_ABS_MASK) >>
>> >                 PORT_HW_CFG_E3_MOD_ABS_SHIFT;
>> >
>> > -           /* Should not happen. This function called upon interrupt
>> > +           /*
>> > +            * Should not happen. This function called upon
>> > + interrupt
>> >              * triggered by GPIO ( since EPIO can only generate interrupts
>> >              * to MCP).
>> >              * So if this function was called and none of the GPIOs was set,
>> > -            * it means the shit hit the fan.
>> >              */
>>
>> It makes the comment ends with a comma, like the end is missing.
>>
>>
>>
>
>Yes, better language would be.
>                /* This should not happen since this function is called
>                 * from interrupt triggered by GPI ..

+1
I've re-worded the last bit.

	/* This should not happen since this function is called
	 * from interrupt triggered by GPIO (since EPIO can only
	 * generate interrupts to MCP).
	 * So if this function was called and none of the GPIOs was set,
	 * it means something disastrous has already happened.
	 */

Thanks!
-Rasesh

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity
  2018-09-19 16:40       ` Mody, Rasesh
@ 2018-10-16 14:45         ` Ferruh Yigit
  2018-10-16 14:52           ` Ferruh Yigit
  0 siblings, 1 reply; 24+ messages in thread
From: Ferruh Yigit @ 2018-10-16 14:45 UTC (permalink / raw)
  To: Mody, Rasesh, Stephen Hemminger, Thomas Monjalon
  Cc: dev, Stephen Hemminger, Patil, Harish

On 9/19/2018 5:40 PM, Mody, Rasesh wrote:
>> From: Stephen Hemminger <stephen@networkplumber.org>
>> Sent: Tuesday, September 18, 2018 8:07 AM
>>
>> On Tue, 18 Sep 2018 11:40:28 +0200
>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>>> 25/07/2018 20:20, Stephen Hemminger:
>>>> No need for profanity in comments.
>>>>
>>>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>>>> ---
>>>>  drivers/net/bnx2x/elink.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bnx2x/elink.c b/drivers/net/bnx2x/elink.c
>>>> index 34a29373af3b..08fe817720a1 100644
>>>> --- a/drivers/net/bnx2x/elink.c
>>>> +++ b/drivers/net/bnx2x/elink.c
>>>> @@ -3993,11 +3993,11 @@ static elink_status_t
>> elink_get_mod_abs_int_cfg(struct bnx2x_softc *sc,
>>>>                        PORT_HW_CFG_E3_MOD_ABS_MASK) >>
>>>>                 PORT_HW_CFG_E3_MOD_ABS_SHIFT;
>>>>
>>>> -           /* Should not happen. This function called upon interrupt
>>>> +           /*
>>>> +            * Should not happen. This function called upon
>>>> + interrupt
>>>>              * triggered by GPIO ( since EPIO can only generate interrupts
>>>>              * to MCP).
>>>>              * So if this function was called and none of the GPIOs was set,
>>>> -            * it means the shit hit the fan.
>>>>              */
>>>
>>> It makes the comment ends with a comma, like the end is missing.
>>>
>>>
>>>
>>
>> Yes, better language would be.
>>                /* This should not happen since this function is called
>>                 * from interrupt triggered by GPI ..
> 
> +1
> I've re-worded the last bit.
> 
> 	/* This should not happen since this function is called
> 	 * from interrupt triggered by GPIO (since EPIO can only
> 	 * generate interrupts to MCP).
> 	 * So if this function was called and none of the GPIOs was set,
> 	 * it means something disastrous has already happened.
> 	 */

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Will use above suggested comment while merging.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity
  2018-10-16 14:45         ` Ferruh Yigit
@ 2018-10-16 14:52           ` Ferruh Yigit
  0 siblings, 0 replies; 24+ messages in thread
From: Ferruh Yigit @ 2018-10-16 14:52 UTC (permalink / raw)
  To: Mody, Rasesh, Stephen Hemminger, Thomas Monjalon
  Cc: dev, Stephen Hemminger, Patil, Harish

On 10/16/2018 3:45 PM, Ferruh Yigit wrote:
> On 9/19/2018 5:40 PM, Mody, Rasesh wrote:
>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>> Sent: Tuesday, September 18, 2018 8:07 AM
>>>
>>> On Tue, 18 Sep 2018 11:40:28 +0200
>>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>>
>>>> 25/07/2018 20:20, Stephen Hemminger:
>>>>> No need for profanity in comments.
>>>>>
>>>>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>>>>> ---
>>>>>  drivers/net/bnx2x/elink.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/bnx2x/elink.c b/drivers/net/bnx2x/elink.c
>>>>> index 34a29373af3b..08fe817720a1 100644
>>>>> --- a/drivers/net/bnx2x/elink.c
>>>>> +++ b/drivers/net/bnx2x/elink.c
>>>>> @@ -3993,11 +3993,11 @@ static elink_status_t
>>> elink_get_mod_abs_int_cfg(struct bnx2x_softc *sc,
>>>>>                        PORT_HW_CFG_E3_MOD_ABS_MASK) >>
>>>>>                 PORT_HW_CFG_E3_MOD_ABS_SHIFT;
>>>>>
>>>>> -           /* Should not happen. This function called upon interrupt
>>>>> +           /*
>>>>> +            * Should not happen. This function called upon
>>>>> + interrupt
>>>>>              * triggered by GPIO ( since EPIO can only generate interrupts
>>>>>              * to MCP).
>>>>>              * So if this function was called and none of the GPIOs was set,
>>>>> -            * it means the shit hit the fan.
>>>>>              */
>>>>
>>>> It makes the comment ends with a comma, like the end is missing.
>>>>
>>>>
>>>>
>>>
>>> Yes, better language would be.
>>>                /* This should not happen since this function is called
>>>                 * from interrupt triggered by GPI ..
>>
>> +1
>> I've re-worded the last bit.
>>
>> 	/* This should not happen since this function is called
>> 	 * from interrupt triggered by GPIO (since EPIO can only
>> 	 * generate interrupts to MCP).
>> 	 * So if this function was called and none of the GPIOs was set,
>> 	 * it means something disastrous has already happened.
>> 	 */
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Will use above suggested comment while merging.

Applied to dpdk-next-net/master, thanks.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 4/4] ixgbe: remove mild profanity
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 4/4] ixgbe: remove mild profanity Stephen Hemminger
@ 2018-10-16 15:00   ` Ferruh Yigit
  0 siblings, 0 replies; 24+ messages in thread
From: Ferruh Yigit @ 2018-10-16 15:00 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dpdk-dev

On 7/25/2018 7:20 PM, stephen at networkplumber.org (Stephen Hemminger) wrote:
> At the tail end of comment about barriers (I feel your pain);
> remove mild profanity.
> 
> Signed-off-by: Stephen Hemminger <sthemmin at microsoft.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 1/4] arm: remove profanity in comment
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 1/4] arm: remove profanity in comment Stephen Hemminger
@ 2018-10-24 23:50   ` Thomas Monjalon
  0 siblings, 0 replies; 24+ messages in thread
From: Thomas Monjalon @ 2018-10-24 23:50 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Stephen Hemminger

25/07/2018 20:20, Stephen Hemminger:
> Update comment to describe the problem better without
> risk of being offensive.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails
  2018-09-18 10:16       ` Burakov, Anatoly
@ 2018-10-24 23:51         ` Thomas Monjalon
  2018-10-25 14:04           ` Burakov, Anatoly
  0 siblings, 1 reply; 24+ messages in thread
From: Thomas Monjalon @ 2018-10-24 23:51 UTC (permalink / raw)
  To: Burakov, Anatoly, Stephen Hemminger; +Cc: dev, Stephen Hemminger

18/09/2018 12:16, Burakov, Anatoly:
> On 18-Sep-18 10:43 AM, Thomas Monjalon wrote:
> > 26/07/2018 11:41, Burakov, Anatoly:
> >> On 25-Jul-18 7:20 PM, Stephen Hemminger wrote:
> >>> There is no need to call rte_exit and crash the application here;
> >>> better to let the application handle the error itself.
> >>>
> >>> Remove the gratuitous profanity which would be visible if
> >>> the rte_exit was still there.
> >>>
> >>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> >>> ---
> >>> --- a/lib/librte_eal/common/eal_common_proc.c
> >>> +++ b/lib/librte_eal/common/eal_common_proc.c
> >>> @@ -841,14 +841,12 @@ 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) {
> >>> +	ret = rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000,
> >>> +				async_reply_handle, pending_req);
> >>> +	if (ret < 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");
> >>
> >> Profanity aside, i think the message was trying to tell me something -
> >> namely, that if alarm_set fails, we're risking to leak this memory if
> >> reply from the peer never comes, and we're risking leaving the
> >> application hanging because the timeout never triggers. I'm not sure if
> >> leaving this "to the user" is the right choice, because there is no way
> >> for the user to free IPC-internal memory if it leaks.
> >>
> >> So i think the proper way to handle this would've been to set the alarm
> >> first, then, if it fails, don't sent the message in the first place.
> > 
> > What should be done here? OK to remove rte_panic for now?
> > 
> 
> As i said, the above fix is wrong because it leaks memory (however 
> unlikely it may be).
> 
> The alarm set call should be moved to before we do send_msg() call (and 
> goto fail; on failure). That way, even if alarm triggers too early (i.e. 
> immediately), the requests tailq will still be locked until we complete 
> our request sends - so we appropriately free memory on response, on 
> timeout or in our failure handler if alarm set has failed.

Someone to fix it, please?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails
  2018-10-24 23:51         ` Thomas Monjalon
@ 2018-10-25 14:04           ` Burakov, Anatoly
  0 siblings, 0 replies; 24+ messages in thread
From: Burakov, Anatoly @ 2018-10-25 14:04 UTC (permalink / raw)
  To: Thomas Monjalon, Stephen Hemminger; +Cc: dev, Stephen Hemminger

On 25-Oct-18 12:51 AM, Thomas Monjalon wrote:
> 18/09/2018 12:16, Burakov, Anatoly:
>> On 18-Sep-18 10:43 AM, Thomas Monjalon wrote:
>>> 26/07/2018 11:41, Burakov, Anatoly:
>>>> On 25-Jul-18 7:20 PM, Stephen Hemminger wrote:
>>>>> There is no need to call rte_exit and crash the application here;
>>>>> better to let the application handle the error itself.
>>>>>
>>>>> Remove the gratuitous profanity which would be visible if
>>>>> the rte_exit was still there.
>>>>>
>>>>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>>>>> ---
>>>>> --- a/lib/librte_eal/common/eal_common_proc.c
>>>>> +++ b/lib/librte_eal/common/eal_common_proc.c
>>>>> @@ -841,14 +841,12 @@ 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) {
>>>>> +	ret = rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000,
>>>>> +				async_reply_handle, pending_req);
>>>>> +	if (ret < 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");
>>>>
>>>> Profanity aside, i think the message was trying to tell me something -
>>>> namely, that if alarm_set fails, we're risking to leak this memory if
>>>> reply from the peer never comes, and we're risking leaving the
>>>> application hanging because the timeout never triggers. I'm not sure if
>>>> leaving this "to the user" is the right choice, because there is no way
>>>> for the user to free IPC-internal memory if it leaks.
>>>>
>>>> So i think the proper way to handle this would've been to set the alarm
>>>> first, then, if it fails, don't sent the message in the first place.
>>>
>>> What should be done here? OK to remove rte_panic for now?
>>>
>>
>> As i said, the above fix is wrong because it leaks memory (however
>> unlikely it may be).
>>
>> The alarm set call should be moved to before we do send_msg() call (and
>> goto fail; on failure). That way, even if alarm triggers too early (i.e.
>> immediately), the requests tailq will still be locked until we complete
>> our request sends - so we appropriately free memory on response, on
>> timeout or in our failure handler if alarm set has failed.
> 
> Someone to fix it, please?
> 

I'll do it.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [dpdk-dev] [PATCH] eal/mp: remove rte_panic and profanity
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails Stephen Hemminger
  2018-07-26  9:34   ` Burakov, Anatoly
  2018-07-26  9:41   ` Burakov, Anatoly
@ 2018-10-26 14:55   ` Anatoly Burakov
  2018-10-26 20:41     ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
  2018-11-13 18:03     ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
  2 siblings, 2 replies; 24+ 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] 24+ messages in thread

* Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity
  2018-07-25 18:20 ` [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity Stephen Hemminger
  2018-09-18  9:40   ` Thomas Monjalon
@ 2018-10-26 14:56   ` Burakov, Anatoly
  1 sibling, 0 replies; 24+ messages in thread
From: Burakov, Anatoly @ 2018-10-26 14:56 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Stephen Hemminger

On 25-Jul-18 7:20 PM, Stephen Hemminger wrote:
> No need for profanity in comments.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>   drivers/net/bnx2x/elink.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/elink.c b/drivers/net/bnx2x/elink.c
> index 34a29373af3b..08fe817720a1 100644
> --- a/drivers/net/bnx2x/elink.c
> +++ b/drivers/net/bnx2x/elink.c
> @@ -3993,11 +3993,11 @@ static elink_status_t elink_get_mod_abs_int_cfg(struct bnx2x_softc *sc,
>   			   PORT_HW_CFG_E3_MOD_ABS_MASK) >>
>   		    PORT_HW_CFG_E3_MOD_ABS_SHIFT;
>   
> -		/* Should not happen. This function called upon interrupt
> +		/*
> +		 * Should not happen. This function called upon interrupt
>   		 * triggered by GPIO ( since EPIO can only generate interrupts
>   		 * to MCP).
>   		 * So if this function was called and none of the GPIOs was set,
> -		 * it means the shit hit the fan.

...then what? :)

>   		 */
>   		if ((cfg_pin < PIN_CFG_GPIO0_P0) ||
>   		    (cfg_pin > PIN_CFG_GPIO3_P1)) {
> 


-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity
  2018-10-26 14:55   ` [dpdk-dev] [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-dev] [PATCH v2] " Anatoly Burakov
  1 sibling, 1 reply; 24+ 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] 24+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity
  2018-10-26 20:41     ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
@ 2018-10-30 10:31       ` Burakov, Anatoly
  0 siblings, 0 replies; 24+ 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] 24+ messages in thread

* [dpdk-dev] [PATCH v2] eal/mp: remove rte_panic and profanity
  2018-10-26 14:55   ` [dpdk-dev] [PATCH] eal/mp: remove rte_panic and profanity Anatoly Burakov
  2018-10-26 20:41     ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
@ 2018-11-13 18:03     ` Anatoly Burakov
  2018-11-13 23:03       ` Thomas Monjalon
  1 sibling, 1 reply; 24+ 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] 24+ messages in thread

* Re: [dpdk-dev] [PATCH v2] eal/mp: remove rte_panic and profanity
  2018-11-13 18:03     ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
@ 2018-11-13 23:03       ` Thomas Monjalon
  0 siblings, 0 replies; 24+ 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] 24+ messages in thread

end of thread, other threads:[~2018-11-13 23:03 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-25 18:20 [dpdk-dev] [PATCH 0/4] small cleanups Stephen Hemminger
2018-07-25 18:20 ` [dpdk-dev] [PATCH 1/4] arm: remove profanity in comment Stephen Hemminger
2018-10-24 23:50   ` Thomas Monjalon
2018-07-25 18:20 ` [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity Stephen Hemminger
2018-09-18  9:40   ` Thomas Monjalon
2018-09-18 15:07     ` Stephen Hemminger
2018-09-19 16:40       ` Mody, Rasesh
2018-10-16 14:45         ` Ferruh Yigit
2018-10-16 14:52           ` Ferruh Yigit
2018-10-26 14:56   ` Burakov, Anatoly
2018-07-25 18:20 ` [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails Stephen Hemminger
2018-07-26  9:34   ` Burakov, Anatoly
2018-07-26  9:41   ` Burakov, Anatoly
2018-09-18  9:43     ` Thomas Monjalon
2018-09-18 10:16       ` Burakov, Anatoly
2018-10-24 23:51         ` Thomas Monjalon
2018-10-25 14:04           ` Burakov, Anatoly
2018-10-26 14:55   ` [dpdk-dev] [PATCH] eal/mp: remove rte_panic and profanity Anatoly Burakov
2018-10-26 20:41     ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2018-10-30 10:31       ` Burakov, Anatoly
2018-11-13 18:03     ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
2018-11-13 23:03       ` Thomas Monjalon
2018-07-25 18:20 ` [dpdk-dev] [PATCH 4/4] ixgbe: remove mild profanity Stephen Hemminger
2018-10-16 15:00   ` Ferruh Yigit

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).