Thread (10 messages) 10 messages, 3 authors, 2021-08-30

AW: [PATCH] can: isotp: omit unintended hrtimer restart on socket release

From: Sven Schuchmann <hidden>
Date: 2021-08-28 13:20:26

Hello,
sorry, I'm late for the party :-) 
But I found that this patch decreases the performance of ISO-TP Stack.

I have created two testscripts where one plays the server and the 
other one is running a test and measuring the time how long
it takes to transfer an ISO-TP Frame with 1000 Bytes.

Without this patch it takes about 35ms to transfer the frame,
with this patch it takes about 145ms over vcan0.

Anyone an idea on this?

Sven


bring up a vcan0 interface with:
sudo modprobe vcan
sudo ip link add dev vcan0 type vcan
sudo ifconfig vcan0 up

here are the scripts:
--- isotp_server.sh ---
#!/bin/bash
iface=vcan0
echo "Wait for Messages on $iface"
while true; do
    exec 3< <(isotprecv -s 77E -d 714 -b F -p AA:AA $iface)
    rxpid=$!
    wait $rxpid
    output=$(cat <&3)
    echo "7F 01 11" | isotpsend -s 77E -d 714 -p AA:AA -L 16:8:0 $iface
done
--- isotp_test.sh ---
#!/bin/bash
databyte=01
numbytes=1000
iface=vcan0
echo "Test iso-tp with $numbytes byte frames on $iface (data:$databyte)"
message="01 " # invalid service 01
for (( i=1; i<=$numbytes; i++ ))
do
   message="$message$databyte "
done
i=1
timemin=10000
timemax=0
timesum=0
while true; do
    ts=$(date +%s%N)
    exec 3< <(isotprecv -s 714 -d 77E -b 5 $iface)
    rxpid=$!
    echo $message | isotpsend -s 714 -d 77E -p AA:AA -L 16:8:0 $iface
    wait $rxpid
    output=$(cat <&3)
    timediff=$((($(date +%s%N) - $ts)/1000000))
    if [ "7F 01 11 " != "$output" ]; then
        echo "ERROR: $i >$output<"
        break
    fi
    if [ $timediff -gt $timemax ]; then
        timemax=$timediff
    fi
    if [ $timediff -lt $timemin ]; then
        timemin=$timediff
    fi
    ((timesum=timesum+timediff))
    timeavg=$(echo "$timesum / $i" | bc -l)
    printf "%5d / curr:%5d / min:%5d / max:%5d / avg:%7.1f\n" $i $timediff $timemin $timemax $timeavg
    ((i=i+1))
done
------

Sven Schuchmann
Schleißheimer Soft- und
Hardwareentwicklung GmbH
Am Kalkofen 10
61206 Nieder-Wöllstadt
GERMANY
Phone: +49 6034 9148 711
Fax: +49 6034 9148 91
Email: schuchmann@schleissheimer.de

Court of Registration: Amtsgericht Friedberg
Registration Number: HRB 1581
Management Board:
Hans-Joachim Schleißheimer
Christine Schleißheimer
quoted hunk ↗ jump to hunk
-----Ursprüngliche Nachricht-----
Von: Oliver Hartkopp [off-list ref]
Gesendet: Freitag, 18. Juni 2021 19:37
An: linux-can@vger.kernel.org
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Betreff: [PATCH] can: isotp: omit unintended hrtimer restart on socket release

When closing the isotp socket the potentially running hrtimers are
canceled before removing the subscription for CAN idendifiers via
can_rx_unregister().

This may lead to an unintended (re)start of a hrtimer in isotp_rcv_cf()
and isotp_rcv_fc() in the case that a CAN frame is received by
isotp_rcv() while the subscription removal is processed.

However, isotp_rcv() is called under RCU protection, so after calling
can_rx_unregister, we may call synchronize_rcu in order to wait for any
RCU read-side critical sections to finish. This prevents the reception
of CAN frames after hrtimer_cancel() and therefore the unintended
(re)start of the hrtimers.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index be6183f8ca11..234cc4ad179a 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1026,13 +1026,10 @@ static int isotp_release(struct socket *sock)
 	list_del(&so->notifier);
 	spin_unlock(&isotp_notifier_lock);

 	lock_sock(sk);

-	hrtimer_cancel(&so->txtimer);
-	hrtimer_cancel(&so->rxtimer);
-
 	/* remove current filters & unregister */
 	if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST))) {
 		if (so->ifindex) {
 			struct net_device *dev;
@@ -1040,14 +1037,18 @@ static int isotp_release(struct socket *sock)
 			if (dev) {
 				can_rx_unregister(net, dev, so->rxid,
 						  SINGLE_MASK(so->rxid),
 						  isotp_rcv, sk);
 				dev_put(dev);
+				synchronize_rcu();
 			}
 		}
 	}

+	hrtimer_cancel(&so->txtimer);
+	hrtimer_cancel(&so->rxtimer);
+
 	so->ifindex = 0;
 	so->bound = 0;

 	sock_orphan(sk);
 	sock->sk = NULL;
--
2.30.2
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help