On 01/03/17(Wed) 18:17, Alexander Bluhm wrote:
> On Tue, Feb 28, 2017 at 01:28:45PM +0100, Martin Pieuchot wrote:
> > Routing sockets do not really need the NET_LOCK(), only route_output()
> > needs it.
>
> That depends what the future use of the netlock will be. route_input()
> is called from tcp, icmp, arp timers. Currently they have kernel
> lock, but maybe we want to switch them to netlock only.
My plan is to reduce the netlock as much as possible. Simply because
holding a rwlock around a huge amount of code is hard to work with and
more likely to generate deadlocks.
In the meantime I'd like to start un-KERNEL_LOCK()ing syscalls operating
on sockets. Based on the socket family it will be more or less easier.
unix sockets, routing sockets and pfkey sockets will be easier to start
with.
The problem we're facing here is that the socket code is common to all
families but can't be protected by the NET_LOCK() because of unix
sockets. But working with unix sockets is hard due to FD passing issues.
So routing sockets are IMHO good candidates.
> Do you see any problems when running this routing code with netlock?
I see a problem with our diagnostic code. What was before a
splsoftassert(IPL_SOFTNET) in sowakeup() is not needed for all types of
sockets. Hence the remaining splsoftnet() in net/rtsock.c are not
needed and I need to get rid of them to move forward.
> > @@ -675,6 +678,7 @@ route_output(struct mbuf *m, ...)
> > * may be not consistent and could cause unexpected behaviour in other
> > * userland clients. Use goto fail instead.
> > */
> > + NET_LOCK(s);
> > switch (rtm->rtm_type) {
> > case RTM_ADD:
> > if (info.rti_info[RTAX_GATEWAY] == NULL) {
>
> We may not sleep here. The malloc(9) in route_output() uses M_NOWAIT,
> so I guess the upper layer assumes that this code does not get
> rescheduled. One idea of netlock is to do the sleeps before the
> atomic sections of the network code.
The idea of the netlock is to not introduce *unsafe* sleeping points.
The malloc(9) above has NOWAIT because of historical reasons. It was
previously using R_Malloc() like all the routing code.
Sleeping here is completely ok. The NET_LOCK() in this function is only
taken to make sure no other thread will try to do a route lookup in
ip_output() while we're messing with the routing table.
Actually the malloc(9) could be converted to M_WAIT like the one in
rt_report() that claudio@ commented like that:
/* XXX why can't we wait? Should be process context... */
rtm = malloc(len, M_RTABLE, M_NOWAIT | M_ZERO);
Since route entry are refcounted it is safe to sleep at this point.
If we want to be picky, the only check that should be moved under
NET_LOCK() is the rtable_exists(9) one. But for now routing table
cannot be deleted, so it is safe.
