Hey all,
looking at the code simplification from Hillf I reworked some of the
cmpxchg() code and removed the old_state mechanism.
I posted a RFC patch here:
https://lore.kernel.org/linux-can/20230331102114.15164-1-socketcan@xxxxxxxxxxxx/
Please comment on the patch whether you think if this could be an
improvement.
Many thanks,
Oliver
On 27.03.23 03:48, Hillf Danton wrote:
On Sun, 26 Mar 2023 18:17:17 +0200 Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
On 26.03.23 13:55, Dae R. Jeong wrote:
If I think correctly, this will make cmpxchg() work, and prevent the
problematic concurrent execution. Could you please check the patch
below?
Hm, interesting idea.
But in which state will so->tx.state be here:
/* wait for complete transmission of current pdu */
err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
if (err)
goto err_out;
Should we better set the tx.state in the error case?
if (err) {
so->tx.state = ISOTP_IDLE;
goto err_out;
}
Best regards,
Oliver
Another 2c only if cmpxchg is preferred.
+++ b/net/can/isotp.c
@@ -932,19 +932,24 @@ static int isotp_sendmsg(struct socket *
return -EADDRNOTAVAIL;
/* we do not support multiple buffers - for now */
- if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE ||
- wq_has_sleeper(&so->wait)) {
- if (msg->msg_flags & MSG_DONTWAIT) {
- err = -EAGAIN;
- goto err_out;
- }
-
+ if (wq_has_sleeper(&so->wait)) {
+ if (msg->msg_flags & MSG_DONTWAIT)
+ return -EAGAIN;
/* wait for complete transmission of current pdu */
err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
if (err)
- goto err_out;
+ return err;
+ }
- so->tx.state = ISOTP_SENDING;
+again:
+ old_state = cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING);
+ if (old_state != ISOTP_IDLE) {
+ if (msg->msg_flags & MSG_DONTWAIT)
+ return -EAGAIN;
+ err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+ if (err)
+ return err;
+ goto again;
}
if (!size || size > MAX_MSG_LENGTH) {
