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@xxxxxxxxxxxxxxxxx
Court of Registration: Amtsgericht Friedberg
Registration Number: HRB 1581
Management Board:
Hans-Joachim Schleißheimer
Christine Schleißheimer
> -----Ursprüngliche Nachricht-----
> Von: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
> Gesendet: Freitag, 18. Juni 2021 19:37
> An: linux-can@xxxxxxxxxxxxxxx
> Cc: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
> ---
> 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
