On 23.01.19 20:30, Kurt Van Dijck wrote:
On wo, 23 jan 2019 13:01:11 +0100, Oleksij Rempel wrote:
This is dangerous behavior, especially if it is done by default. If we
still needed it, it can be added after upstreaming.
In a true (?) j1939 program, you don't care about the precise packet size
you're sending. If you then send 2 subsequent packets, 1 with TP and 1
without, then the 2nd will likely arrive sooner than the first, which
is probably not what you want.
The piece of code you're about to remove increments will set
jsk->skb_pending only if it was 0, atomically.
If it wasn't 0, then send() will block or fail.
Can you explain how this is dangerous?
IMHO, it is not done by default, but ony when MSG_SYN is given during send().
How do you think that synchronous send is done by default?
With the code you are describing, the behavior of send() is broken.
a sequence of send call will be pushed to the bus in unpredictable order. And to get it in
predicable order we should run in full blocking mode.
IMHO, proper way:
- implement internal queue to guarantee the order of packets within one socket.
- if packet should be run without order of with different PRIO, send it over different socket.
this way allows to run ordered async mode. which is a save, predictable default.
Kurt
Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
---
net/can/j1939/socket.c | 33 +--------------------------------
1 file changed, 1 insertion(+), 32 deletions(-)
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index a313077ca80a..38e2fb47292b 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -73,23 +73,6 @@ static inline bool j1939_pgn_is_clean_pdu(pgn_t pgn)
return true;
}
-/* j1939_sock_pending_add_first
- * Succeeds when the first pending SKB is scheduled
- * Fails when SKB are already pending
- */
-static inline int j1939_sock_pending_add_first(struct sock *sk)
-{
- struct j1939_sock *jsk = j1939_sk(sk);
-
- /* atomic_cmpxchg returns the old value
- * When it was 0, it is exchanged with 1 and this function
- * succeeded. (return 1)
- * When it was != 0, it is not exchanged, and this function
- * fails (returns 0).
- */
- return !atomic_cmpxchg(&jsk->skb_pending, 0, 1);
-}
-
static inline void j1939_sock_pending_add(struct sock *sk)
{
struct j1939_sock *jsk = j1939_sk(sk);
@@ -773,25 +756,11 @@ static int j1939_sk_sendmsg(struct socket *sock, struct msghdr *msg,
if (!ndev)
return -ENXIO;
- if (msg->msg_flags & MSG_SYN) {
- if (msg->msg_flags & MSG_DONTWAIT) {
- ret = j1939_sock_pending_add_first(&jsk->sk);
- if (ret > 0)
- ret = -EAGAIN;
- } else {
- ret = wait_event_interruptible(jsk->waitq,
- j1939_sock_pending_add_first(&jsk->sk));
- }
- if (ret < 0)
- goto put_dev;
- } else {
- j1939_sock_pending_add(&jsk->sk);
- }
-
priv = j1939_priv_get_by_ndev(ndev);
if (!priv)
return -EINVAL;
+ j1939_sock_pending_add(&jsk->sk);
if (size > 8)
/* re-route via transport protocol */
ret = j1939_sk_send_multi(priv, sk, msg, size);
--
2.20.1
Kind regards,
Oleksij Rempel
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
