On Thu, Jan 24, 2019 at 07:58:35PM +0100, Kurt Van Dijck wrote:
> On do, 24 jan 2019 09:24:06 +0100, Oleksij Rempel wrote:
> >
> > 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.
>
> This part
> > >>- ret = wait_event_interruptible(jsk->waitq,
> > >>- j1939_sock_pending_add_first(&jsk->sk));
> implements the 'true blocking' mode, it waits until the previous one has
> been sent.
> MSG_DONTWAIT returns -EAGAIN if it is not first
> or at least, it should behave so.
> Looking at the code, it seems that the latest version has the check
> inverted.
> > >>- if (msg->msg_flags & MSG_DONTWAIT) {
> > >>- ret = j1939_sock_pending_add_first(&jsk->sk);
> > >>- if (ret > 0)
> > >>- ret = -EAGAIN;
> Strange, it once worked. I'm not in the possibility to verify it.
>
> >
> > IMHO, proper way:
> > - implement internal queue to guarantee the order of packets within one socket.
> I learned this thing: avoid to be intelligent in kernel. do exactly what
> you're asked for and fail if the slightest thing go wrong, so userspace
> can do something smart with it.
this is exactly current plan for this socket:
step 1, implement full blocking mode with proper session return error code
step 2, implement session status update over MSR_ERRORQUEUE
with steps 1 and 2 we should have needed infrastructure to keep the
order of send packets. Kernel makes no smart decisions for you, if user
will set MSG_DONTWAIT and there is space in the queue, take the package
and wait until it can be send.
If no flags is set and socket is already busy with sending an (E)TP
session, then block until session is completed or aborted.
It sound simple and predictable for me. Or am I missing some thing?
> > - if packet should be run without order of with different PRIO, send it over different socket.
> True.
> It makes little sense to mix MSG_SYN with !MSG_SYN on 1 socket,
> but 'not being smart' isn't a real argument to forbid it either.
>
> Kind regards,
> Kurt
>
--
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 |
