Hello,
On Thu, 2 Mar 2000 kuznet@ms2.inr.ac.ru wrote:
> Hello!
>
> > Here is a list of proposed changes in the SIGIO handling for
> > sockets.
>
> I see several evident small holes in the patch, which make it inacceptable.
>
> 1.
> + /* We want to modify the accepted socket, so check accept_flags */
> + if (newsock->sk->accept_flags)
> + check_accept_flags(err, sock, newsock);
>
> It cannot work. Please, read comment above sock_map_fd().
Yep. I understand. Is it possible some extra checks/locks
to ensure that we don't modify already closed file? I know that some
locking is needed and I need your help if this idea will be
accepted.
>
> 2. sk->state_change() cannot be replaced with sk->data_ready()
> even when it sends plain POLL_IN. They are different.
> I did this mistake once and had to return them back. 8)
the code
sk->state_change(sk);
sock_wake_async(sk->socket, 1, POLL_HUP);
looks very similar to this code
sk->data_ready(sk,0);
except that the optional event is POLL_IN instead of
POLL_HUP and sock_wake_async is not guarded by callback_lock,
here in tcp_fin(). I assume the lock is not required in this
context. May be I'm wrong, I compare sock_def_wakeup() and
sock_def_readable(). But if we don't use POLL_HUP as an
end marker this change must be removed.
>
> 3. Patch is not formatted, which shows that you did not look at it.
> Also, //. 8)8)
Sorry, I propose it for testing and not as a patch
ready for the mainstream kernel. I looked in it but I thought
that these parts will be corrected later. // just notes the
change which can be restored.
>
> 4. POLL_HUP cannot be used as barrier for file close. It is _evident_,
> that it has no relation to the recent discussion in the list.
>
> If you follow the discussion, you send rather POLLNVAL event. See?
> And not from network but from the point where file _descriptor_ is
> invalidated. So, new signal sources are simply in wrong place and
> wrongly named.
My error in sock_release is obvious. The locks must be
removed. My tests worked without locks but I added them later.
sock->sk is always NULL after sock->ops->release(sock).
>
> Besides that, please, make sure that such events are _not_ sent
> with not-rt signals, when signals are not queued. I see, you forgot
> that signal is more expensive than syscall, it was the reason
> why old signals aggresively compressed events.
To be honest, I don't know it. But my SIGRTMIN is blocked
and I dequeue the events using sigwaitinfo(). I noted that the
signal from close() can be removed and the application can use
FASYNC=off when the end marker is needed to follow the unexpected
events. In this case we don't lose anything.
>
> > - Don't return POLL_HUP at EOF, return POLL_IN instead.
>
> POLL_HUP had well-defined semantics. Please, do not break it,
> if it is possible.
The semantic is not changed for poll() but is different
from the rt events (si_code). But I agree if the new POLL_xxx is
added (in include/asm/siginfo.h) not to change the POLL_HUP
semantic. We just need a POLL_xxx for an end marker. These
POLL_xxx codes (include/asm/siginfo.h) are different from POLLxxx
which are defined in include/asm/poll.h. There is a table
"band_table" that converts POLL_xxx to POLLxxx in fs/fcntl.c.
I rely on it when changing the POLL_HUP semantic.
>
>
> > - add SO_ACCEPT_FLAGS socket option.
>
> I would prefer acceptx() syscall, which will make this and
> receive the first chunk of data on demand.
OK, the POLL_IN event generated from accept*() can
save one syscall. What about the inheritance of some flags?
The current inheritance is a minimum. Some people proposed
more flags to be inherited. They can be added in
check_accept_flags. Or one combo syscall can do the job.
>
> But generally I suspect SO_ACCEPT_FLAGS is a broken idea.
> You work with VFS object, but control it from socket layer.
> You forgot about dup*(), by the way.
Yep. It looks ugly. May be the file counters must be
checked. I just followed the current handling of closing
the file descriptors without checking sure
> that it is right way? 8)8)8) I see, the patch consists of two
> independant parts: accept() extension looks OK (provided
> it is made correctly) and close tracking. Close tracking
> is fully broken, it must be hooked to VFS in the places, where
> descriptor is invalidated.
I agree to remove the event from close() if FASYNC=off
sends the end marker. Yes, if we provide end marker for all
async file descriptors, this is a more complex task. All
protocols must be corrected. So, I prefer close() not to
send the end marker because this can lead to unconditional
notifications from each close (if put in the VFS layers) which
can cost extra syscalls to dequeue these events when they
are not always needed.
I saved 4 syscalls (3 fcntl + 1 read) and I use two
extra syscalls to read the end marker using sigwaitinfo (if
send from FASYNC=off). But this is another issue. If the event
is removed from close() and the end marker is not used (via
FASYNC=off) we don't receive more events than before. The
end marker is optional and can be generated from the
application when needed. Normally, we don't use FASYNC=off
but this is the point where the events are stopped and we
can safely send the last event.
>
> > Are there any plans SIGIO to be supported for AF_UNIX?
>
> Khm... Is it really not supported?
I don't receive rt events for AF_UNIX sockets. I have to
check it again.
>
> Alexey
>
Thank you for your very fast response! I know that
this patch is not ready nor fully functional but it contains
the basics. It can be tuned. I'll remove the event from
close(). With this patch I just want to show an example
for some of the ideas. I can't solve this complex task alone
and I expect that more people will be interested.
Regards
-- Julian Anastasov <uli@linux.tu-varna.acad.bg>- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Tue Mar 07 2000 - 21:00:13 EST