Re: Fewer KERNEL_LOCK() for IPsec
On Sun, May 28, 2017 at 04:06:22PM +0200, Martin Pieuchot wrote:
> Our original plan was to unlock the forwarding path without taking any
> lock. That's why I added some KERNEL_LOCK()/KERNEL_UNLOCK() around non
> MP-safe data structures.
>
> Now we're going to rely on the NET_LOCK() for the interface address and
> multicast list. So replace the KERNEL_LOCK()/KERNEL_UNLOCK() their.
>
> IPsec will still need the KERNEL_LOCK(). But taking and releasing it
> doesn't make sense. It's also wrong since tdb are not refcounted. So
> assert that the KERNEL_LOCK() is held. The idea is to run the softnet
> task under KERNEL_LOCK() if ipsec_in_use is true.
>
> ok?
OK bluhm@
>
> Index: netinet/in.c
> ===
> RCS file: /cvs/src/sys/netinet/in.c,v
> retrieving revision 1.138
> diff -u -p -u -4 -r1.138 in.c
> --- netinet/in.c 16 May 2017 12:24:01 - 1.138
> +++ netinet/in.c 28 May 2017 14:00:12 -
> @@ -797,10 +797,8 @@ in_addmulti(struct in_addr *ap, struct i
> {
> struct in_multi *inm;
> struct ifreq ifr;
>
> - NET_ASSERT_LOCKED();
> -
> /*
>* See if address already in list.
>*/
> IN_LOOKUP_MULTI(*ap, ifp, inm);
> @@ -900,12 +898,10 @@ in_hasmulti(struct in_addr *ap, struct i
> {
> struct in_multi *inm;
> int joined;
>
> - KERNEL_LOCK();
> IN_LOOKUP_MULTI(*ap, ifp, inm);
> joined = (inm != NULL);
> - KERNEL_UNLOCK();
>
> return (joined);
> }
>
> Index: netinet/in_var.h
> ===
> RCS file: /cvs/src/sys/netinet/in_var.h,v
> retrieving revision 1.39
> diff -u -p -u -4 -r1.39 in_var.h
> --- netinet/in_var.h 15 Jun 2016 19:39:34 - 1.39
> +++ netinet/in_var.h 28 May 2017 14:01:33 -
> @@ -85,8 +85,9 @@ struct in_aliasreq {
> /* struct ifnet *ifp; */\
> /* struct in_ifaddr *ia; */ \
> do { \
> struct ifaddr *ifa; \
> + NET_ASSERT_LOCKED();\
> TAILQ_FOREACH(ifa, &(ifp)->if_addrlist, ifa_list) { \
> if (ifa->ifa_addr->sa_family == AF_INET)\
> break; \
> } \
> @@ -141,8 +142,9 @@ ifmatoinm(struct ifmaddr *ifma)
> do { \
> struct ifmaddr *ifma; \
> \
> (inm) = NULL; \
> + NET_ASSERT_LOCKED();\
> TAILQ_FOREACH(ifma, &(ifp)->if_maddrlist, ifma_list)\
> if (ifma->ifma_addr->sa_family == AF_INET &&\
> ifmatoinm(ifma)->inm_addr.s_addr == (addr).s_addr) {\
> (inm) = ifmatoinm(ifma);\
> Index: netinet/ip_input.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.306
> diff -u -p -u -4 -r1.306 ip_input.c
> --- netinet/ip_input.c28 May 2017 12:22:54 - 1.306
> +++ netinet/ip_input.c28 May 2017 13:56:09 -
> @@ -440,11 +440,11 @@ ipv4_input(struct mbuf *m)
> #ifdef IPSEC
> if (ipsec_in_use) {
> int rv;
>
> - KERNEL_LOCK();
> + KERNEL_ASSERT_LOCKED();
> +
> rv = ipsec_forward_check(m, hlen, AF_INET);
> - KERNEL_UNLOCK();
> if (rv != 0) {
> ipstat_inc(ips_cantforward);
> goto bad;
> }
> @@ -666,9 +666,9 @@ in_ouraddr(struct mbuf *m, struct ifnet
>* The check in the loop assumes you only rx a packet on an UP
>* interface, and that M_BCAST will only be set on a BROADCAST
>* interface.
>*/
> - KERNEL_LOCK();
> + NET_ASSERT_LOCKED();
> TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
> if (ifa->ifa_addr->sa_family != AF_INET)
> continue;
>
> @@ -677,9 +677,8 @@ in_ouraddr(struct mbuf *m, struct ifnet
> match = 1;
> break;
> }
> }
> - KERNEL_UNLOCK();
> }
>
> return (match);
> }
> Index: netinet/ip_output.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> retrieving revision
Fewer KERNEL_LOCK() for IPsec
Our original plan was to unlock the forwarding path without taking any
lock. That's why I added some KERNEL_LOCK()/KERNEL_UNLOCK() around non
MP-safe data structures.
Now we're going to rely on the NET_LOCK() for the interface address and
multicast list. So replace the KERNEL_LOCK()/KERNEL_UNLOCK() their.
IPsec will still need the KERNEL_LOCK(). But taking and releasing it
doesn't make sense. It's also wrong since tdb are not refcounted. So
assert that the KERNEL_LOCK() is held. The idea is to run the softnet
task under KERNEL_LOCK() if ipsec_in_use is true.
ok?
Index: netinet/in.c
===
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.138
diff -u -p -u -4 -r1.138 in.c
--- netinet/in.c16 May 2017 12:24:01 - 1.138
+++ netinet/in.c28 May 2017 14:00:12 -
@@ -797,10 +797,8 @@ in_addmulti(struct in_addr *ap, struct i
{
struct in_multi *inm;
struct ifreq ifr;
- NET_ASSERT_LOCKED();
-
/*
* See if address already in list.
*/
IN_LOOKUP_MULTI(*ap, ifp, inm);
@@ -900,12 +898,10 @@ in_hasmulti(struct in_addr *ap, struct i
{
struct in_multi *inm;
int joined;
- KERNEL_LOCK();
IN_LOOKUP_MULTI(*ap, ifp, inm);
joined = (inm != NULL);
- KERNEL_UNLOCK();
return (joined);
}
Index: netinet/in_var.h
===
RCS file: /cvs/src/sys/netinet/in_var.h,v
retrieving revision 1.39
diff -u -p -u -4 -r1.39 in_var.h
--- netinet/in_var.h15 Jun 2016 19:39:34 - 1.39
+++ netinet/in_var.h28 May 2017 14:01:33 -
@@ -85,8 +85,9 @@ structin_aliasreq {
/* struct ifnet *ifp; */\
/* struct in_ifaddr *ia; */ \
do { \
struct ifaddr *ifa; \
+ NET_ASSERT_LOCKED();\
TAILQ_FOREACH(ifa, &(ifp)->if_addrlist, ifa_list) { \
if (ifa->ifa_addr->sa_family == AF_INET)\
break; \
} \
@@ -141,8 +142,9 @@ ifmatoinm(struct ifmaddr *ifma)
do { \
struct ifmaddr *ifma; \
\
(inm) = NULL; \
+ NET_ASSERT_LOCKED();\
TAILQ_FOREACH(ifma, &(ifp)->if_maddrlist, ifma_list)\
if (ifma->ifma_addr->sa_family == AF_INET &&\
ifmatoinm(ifma)->inm_addr.s_addr == (addr).s_addr) {\
(inm) = ifmatoinm(ifma);\
Index: netinet/ip_input.c
===
RCS file: /cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.306
diff -u -p -u -4 -r1.306 ip_input.c
--- netinet/ip_input.c 28 May 2017 12:22:54 - 1.306
+++ netinet/ip_input.c 28 May 2017 13:56:09 -
@@ -440,11 +440,11 @@ ipv4_input(struct mbuf *m)
#ifdef IPSEC
if (ipsec_in_use) {
int rv;
- KERNEL_LOCK();
+ KERNEL_ASSERT_LOCKED();
+
rv = ipsec_forward_check(m, hlen, AF_INET);
- KERNEL_UNLOCK();
if (rv != 0) {
ipstat_inc(ips_cantforward);
goto bad;
}
@@ -666,9 +666,9 @@ in_ouraddr(struct mbuf *m, struct ifnet
* The check in the loop assumes you only rx a packet on an UP
* interface, and that M_BCAST will only be set on a BROADCAST
* interface.
*/
- KERNEL_LOCK();
+ NET_ASSERT_LOCKED();
TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
if (ifa->ifa_addr->sa_family != AF_INET)
continue;
@@ -677,9 +677,8 @@ in_ouraddr(struct mbuf *m, struct ifnet
match = 1;
break;
}
}
- KERNEL_UNLOCK();
}
return (match);
}
Index: netinet/ip_output.c
===
RCS file: /cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.339
diff -u -p -u -4 -r1.339 ip_output.c
--- netinet/ip_output.c 19 Apr 2017 15:21:54 - 1.339
+++ netinet/ip_output.c 28 May 2017 13:58:03 -
@@ -191,13 +191,11 @@ reroute:
