[dpdk-dev] [PATCH 0/4] small cleanups

[dpdk-dev] [PATCH 0/4] small cleanups

[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

[dpdk-dev] [PATCH 1/4] arm: remove profanity in comment

[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

[dpdk-dev] [PATCH 2/4] bnx2x: remove profanity

[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

[dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails

[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

[dpdk-dev] [PATCH 4/4] ixgbe: remove mild profanity

[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

Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails

[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>
> ---

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

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails

[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>
> ---
>   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

Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity

[Thomas Monjalon]

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.

Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails

[Thomas Monjalon]

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?

Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails

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

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity

[Stephen Hemminger]

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

Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity

[Mody, Rasesh]

>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

Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity

[Ferruh Yigit]

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.

Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity

[Ferruh Yigit]

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.

Re: [dpdk-dev] [PATCH 4/4] ixgbe: remove mild profanity

[Ferruh Yigit]

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.

Re: [dpdk-dev] [PATCH 1/4] arm: remove profanity in comment

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails

[Thomas Monjalon]

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?

Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails

[Burakov, Anatoly]

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

[dpdk-dev] [PATCH] eal/mp: remove rte_panic and profanity

[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>
---
 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

Re: [dpdk-dev] [PATCH 2/4] bnx2x: remove profanity

[Burakov, Anatoly]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity

[Thomas Monjalon]

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.

Re: [dpdk-dev] [dpdk-stable] [PATCH] eal/mp: remove rte_panic and profanity

[Burakov, Anatoly]

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

[dpdk-dev] [PATCH v2] eal/mp: remove rte_panic and profanity

[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>
---

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

Re: [dpdk-dev] [PATCH v2] eal/mp: remove rte_panic and profanity

[Thomas Monjalon]

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