[PATCH 1/3] net/cap: fix non-rt signal overflow

[PATCH 1/3] net/cap: fix non-rt signal overflow

[Philippe Gerum]

From: Philippe Gerum <rpm@xenomai.org>

rtdm_nrtsig_pend() is based on ipipe_post_root_work(), which keeps a
copy of every request descriptor internally until it is consumed by
the secondary mode handler. Triggering rtdm_nrtsig_pend() every time a
packet is relayed from the stack to the tap device may cause such
request buffer to overflow under pressure.

To address this issue, trigger the non-rt signal only when the RX
queue transitions from empty to non-empty state as a result of
enqueuing the next skb.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/net/addons/cap.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/kernel/drivers/net/addons/cap.c b/kernel/drivers/net/addons/cap.c
index 5f83e19e3..293fcc84b 100644
--- a/kernel/drivers/net/addons/cap.c
+++ b/kernel/drivers/net/addons/cap.c
@@ -59,6 +59,7 @@ void rtcap_rx_hook(struct rtskb *rtskb)
 {
 	int                     ifindex;
 	int                     active;
+	bool			trigger = false;
 
 	if (rtskb->cap_flags & RTSKB_CAP_SHARED)
 		return;
@@ -81,9 +82,10 @@ void rtcap_rx_hook(struct rtskb *rtskb)
 		return;
 	}
 
-	if (cap_queue.first == NULL)
+	if (cap_queue.first == NULL) {
 		cap_queue.first = rtskb;
-	else
+		trigger = true;
+	} else
 		cap_queue.last->cap_next = rtskb;
 	cap_queue.last = rtskb;
 	rtskb->cap_next = NULL;
@@ -91,13 +93,15 @@ void rtcap_rx_hook(struct rtskb *rtskb)
 	rtskb->cap_flags |= RTSKB_CAP_SHARED;
 	rtskb->cap_dev = rtskb->rtdev;
 
-	rtdm_nrtsig_pend(&cap_signal);
+	if (trigger)
+		rtdm_nrtsig_pend(&cap_signal);
 }
 
 int rtcap_xmit_hook(struct rtskb *rtskb, struct rtnet_device *rtdev)
 {
 	struct tap_device_t *tap_dev = &tap_device[rtskb->rtdev->ifindex];
 	rtdm_lockctx_t context;
+	bool trigger = false;
 	int active;
 
 	active = tap_dev->present & XMIT_HOOK;
@@ -121,15 +125,17 @@ int rtcap_xmit_hook(struct rtskb *rtskb, struct rtnet_device *rtdev)
 
 	rtdm_lock_get_irqsave(&rtcap_lock, context);
 
-	if (cap_queue.first == NULL)
+	if (cap_queue.first == NULL) {
 		cap_queue.first = rtskb;
-	else
+		trigger = true;
+	} else
 		cap_queue.last->cap_next = rtskb;
 	cap_queue.last = rtskb;
 
 	rtdm_lock_put_irqrestore(&rtcap_lock, context);
 
-	rtdm_nrtsig_pend(&cap_signal);
+	if (trigger)
+		rtdm_nrtsig_pend(&cap_signal);
 done:
 	return tap_dev->orig_xmit(rtskb, rtdev);
 }
-- 
2.21.0

[PATCH 2/3] net/rtmac: vnic: fix non-rt signal overflow

[Philippe Gerum]

From: Philippe Gerum <rpm@xenomai.org>

rtdm_nrtsig_pend() is based on ipipe_post_root_work(), which keeps a
copy of every request descriptor internally until it is consumed by
the secondary mode handler. Triggering rtdm_nrtsig_pend() every time a
packet is relayed from the stack to the VNIC may cause such request
buffer to overflow under pressure.

To address this issue, trigger the non-rt signal only when the RX
queue transitions from empty to non-empty state as a result of
enqueuing the next skb.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/net/stack/rtmac/rtmac_vnic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/drivers/net/stack/rtmac/rtmac_vnic.c b/kernel/drivers/net/stack/rtmac/rtmac_vnic.c
index 85ee35002..138e276ad 100644
--- a/kernel/drivers/net/stack/rtmac/rtmac_vnic.c
+++ b/kernel/drivers/net/stack/rtmac/rtmac_vnic.c
@@ -52,8 +52,8 @@ int rtmac_vnic_rx(struct rtskb *rtskb, u16 type)
 
 	rtskb->protocol = type;
 
-	rtskb_queue_tail(&rx_queue, rtskb);
-	rtdm_nrtsig_pend(&vnic_signal);
+	if (rtskb_queue_tail_check(&rx_queue, rtskb))
+		rtdm_nrtsig_pend(&vnic_signal);
 
 	return 0;
 }
-- 
2.21.0

[PATCH 3/3] net/rtpc: fix non-rt signal overflow

[Philippe Gerum]

From: Philippe Gerum <rpm@xenomai.org>

rtdm_nrtsig_pend() is based on ipipe_post_root_work(), which keeps a
copy of every request descriptor internally until it is consumed by
the secondary mode handler. Triggering rtdm_nrtsig_pend() every time a
packet is relayed from the stack to the tap device may cause such
request buffer to overflow under pressure.

To address this issue, trigger the non-rt signal only when the call
completion queue transitions from empty to non-empty state.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/net/stack/rtnet_rtpc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/drivers/net/stack/rtnet_rtpc.c b/kernel/drivers/net/stack/rtnet_rtpc.c
index 4fcf5589e..cd5f0546d 100644
--- a/kernel/drivers/net/stack/rtnet_rtpc.c
+++ b/kernel/drivers/net/stack/rtnet_rtpc.c
@@ -146,12 +146,15 @@ static inline struct rt_proc_call *rtpc_dequeue_pending_call(void)
 static inline void rtpc_queue_processed_call(struct rt_proc_call *call)
 {
 	rtdm_lockctx_t context;
+	bool trigger;
 
 	rtdm_lock_get_irqsave(&processed_calls_lock, context);
+	trigger = list_empty(&processed_calls);
 	list_add_tail(&call->list_entry, &processed_calls);
 	rtdm_lock_put_irqrestore(&processed_calls_lock, context);
 
-	rtdm_nrtsig_pend(&rtpc_nrt_signal);
+	if (trigger)
+		rtdm_nrtsig_pend(&rtpc_nrt_signal);
 }
 
 static inline struct rt_proc_call *rtpc_dequeue_processed_call(void)
-- 
2.21.0

Re: [PATCH 1/3] net/cap: fix non-rt signal overflow

[Jan Kiszka]

On 30.11.19 19:39, Philippe Gerum via Xenomai wrote:
> From: Philippe Gerum <rpm@xenomai.org>
> 
> rtdm_nrtsig_pend() is based on ipipe_post_root_work(), which keeps a
> copy of every request descriptor internally until it is consumed by
> the secondary mode handler. Triggering rtdm_nrtsig_pend() every time a
> packet is relayed from the stack to the tap device may cause such
> request buffer to overflow under pressure.
> 
> To address this issue, trigger the non-rt signal only when the RX
> queue transitions from empty to non-empty state as a result of
> enqueuing the next skb.
> 
> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
> ---
>   kernel/drivers/net/addons/cap.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/drivers/net/addons/cap.c b/kernel/drivers/net/addons/cap.c
> index 5f83e19e3..293fcc84b 100644
> --- a/kernel/drivers/net/addons/cap.c
> +++ b/kernel/drivers/net/addons/cap.c
> @@ -59,6 +59,7 @@ void rtcap_rx_hook(struct rtskb *rtskb)
>   {
>   	int                     ifindex;
>   	int                     active;
> +	bool			trigger = false;
>   
>   	if (rtskb->cap_flags & RTSKB_CAP_SHARED)
>   		return;
> @@ -81,9 +82,10 @@ void rtcap_rx_hook(struct rtskb *rtskb)
>   		return;
>   	}
>   
> -	if (cap_queue.first == NULL)
> +	if (cap_queue.first == NULL) {
>   		cap_queue.first = rtskb;
> -	else
> +		trigger = true;
> +	} else
>   		cap_queue.last->cap_next = rtskb;
>   	cap_queue.last = rtskb;
>   	rtskb->cap_next = NULL;
> @@ -91,13 +93,15 @@ void rtcap_rx_hook(struct rtskb *rtskb)
>   	rtskb->cap_flags |= RTSKB_CAP_SHARED;
>   	rtskb->cap_dev = rtskb->rtdev;
>   
> -	rtdm_nrtsig_pend(&cap_signal);
> +	if (trigger)
> +		rtdm_nrtsig_pend(&cap_signal);
>   }
>   
>   int rtcap_xmit_hook(struct rtskb *rtskb, struct rtnet_device *rtdev)
>   {
>   	struct tap_device_t *tap_dev = &tap_device[rtskb->rtdev->ifindex];
>   	rtdm_lockctx_t context;
> +	bool trigger = false;
>   	int active;
>   
>   	active = tap_dev->present & XMIT_HOOK;
> @@ -121,15 +125,17 @@ int rtcap_xmit_hook(struct rtskb *rtskb, struct rtnet_device *rtdev)
>   
>   	rtdm_lock_get_irqsave(&rtcap_lock, context);
>   
> -	if (cap_queue.first == NULL)
> +	if (cap_queue.first == NULL) {
>   		cap_queue.first = rtskb;
> -	else
> +		trigger = true;
> +	} else
>   		cap_queue.last->cap_next = rtskb;
>   	cap_queue.last = rtskb;
>   
>   	rtdm_lock_put_irqrestore(&rtcap_lock, context);
>   
> -	rtdm_nrtsig_pend(&cap_signal);
> +	if (trigger)
> +		rtdm_nrtsig_pend(&cap_signal);
>   done:
>   	return tap_dev->orig_xmit(rtskb, rtdev);
>   }
> 

Doesn't apply to next - what's your baseline?

Taking patch 2 and 3 meanwhile.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 1/3] net/cap: fix non-rt signal overflow

[Philippe Gerum]

On 12/2/19 7:46 AM, Jan Kiszka wrote:
> On 30.11.19 19:39, Philippe Gerum via Xenomai wrote:
>> From: Philippe Gerum <rpm@xenomai.org>
>>
>> rtdm_nrtsig_pend() is based on ipipe_post_root_work(), which keeps a
>> copy of every request descriptor internally until it is consumed by
>> the secondary mode handler. Triggering rtdm_nrtsig_pend() every time a
>> packet is relayed from the stack to the tap device may cause such
>> request buffer to overflow under pressure.
>>
>> To address this issue, trigger the non-rt signal only when the RX
>> queue transitions from empty to non-empty state as a result of
>> enqueuing the next skb.
>>
>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>> ---
>>   kernel/drivers/net/addons/cap.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/drivers/net/addons/cap.c b/kernel/drivers/net/addons/cap.c
>> index 5f83e19e3..293fcc84b 100644
>> --- a/kernel/drivers/net/addons/cap.c
>> +++ b/kernel/drivers/net/addons/cap.c
>> @@ -59,6 +59,7 @@ void rtcap_rx_hook(struct rtskb *rtskb)
>>   {
>>       int                     ifindex;
>>       int                     active;
>> +    bool            trigger = false;
>>         if (rtskb->cap_flags & RTSKB_CAP_SHARED)
>>           return;
>> @@ -81,9 +82,10 @@ void rtcap_rx_hook(struct rtskb *rtskb)
>>           return;
>>       }
>>   -    if (cap_queue.first == NULL)
>> +    if (cap_queue.first == NULL) {
>>           cap_queue.first = rtskb;
>> -    else
>> +        trigger = true;
>> +    } else
>>           cap_queue.last->cap_next = rtskb;
>>       cap_queue.last = rtskb;
>>       rtskb->cap_next = NULL;
>> @@ -91,13 +93,15 @@ void rtcap_rx_hook(struct rtskb *rtskb)
>>       rtskb->cap_flags |= RTSKB_CAP_SHARED;
>>       rtskb->cap_dev = rtskb->rtdev;
>>   -    rtdm_nrtsig_pend(&cap_signal);
>> +    if (trigger)
>> +        rtdm_nrtsig_pend(&cap_signal);
>>   }
>>     int rtcap_xmit_hook(struct rtskb *rtskb, struct rtnet_device *rtdev)
>>   {
>>       struct tap_device_t *tap_dev = &tap_device[rtskb->rtdev->ifindex];
>>       rtdm_lockctx_t context;
>> +    bool trigger = false;
>>       int active;
>>         active = tap_dev->present & XMIT_HOOK;
>> @@ -121,15 +125,17 @@ int rtcap_xmit_hook(struct rtskb *rtskb, struct rtnet_device *rtdev)
>>         rtdm_lock_get_irqsave(&rtcap_lock, context);
>>   -    if (cap_queue.first == NULL)
>> +    if (cap_queue.first == NULL) {
>>           cap_queue.first = rtskb;
>> -    else
>> +        trigger = true;
>> +    } else
>>           cap_queue.last->cap_next = rtskb;
>>       cap_queue.last = rtskb;
>>         rtdm_lock_put_irqrestore(&rtcap_lock, context);
>>   -    rtdm_nrtsig_pend(&cap_signal);
>> +    if (trigger)
>> +        rtdm_nrtsig_pend(&cap_signal);
>>   done:
>>       return tap_dev->orig_xmit(rtskb, rtdev);
>>   }
>>
> 
> Doesn't apply to next - what's your baseline?
> 

Mm, sorry. That one went on top of the multicast+VLAN support which is pending in my rtnet queue. A patch rebased on -next is on its way.

-- 
Philippe.