Re: in{,6}_hasmulti() for MP
On 15/01/16(Fri) 12:00, Martin Pieuchot wrote:
> One of the checks missing to have an unlocked forwarding path is related
> to multicast. We must ensure that the list of multicast groups attached
> to an ifp is not modified when the CPU processing a packet is traversing
> it.
>
> In order to prepare for such change the diff below introduces a new
> function to abstract the list traversal.
>
> It is a simple refactoring but multiples reviews are more than welcome
> at this stage of the release cycle.
>
> ok?
I'm still looking for reviews.
> Index: netinet/in.c
> ===
> RCS file: /cvs/src/sys/netinet/in.c,v
> retrieving revision 1.125
> diff -u -p -r1.125 in.c
> --- netinet/in.c 3 Dec 2015 21:57:59 - 1.125
> +++ netinet/in.c 15 Jan 2016 10:33:36 -
> @@ -880,6 +880,21 @@ in_delmulti(struct in_multi *inm)
> }
> }
>
> +/*
> + * Return 1 if the multicast group represented by ``ap'' has been
> + * joined by interface ``ifp'', 0 otherwise.
> + */
> +int
> +in_hasmulti(struct in_addr *ap, struct ifnet *ifp)
> +{
> + struct in_multi *inm;
> + int joined;
> +
> + IN_LOOKUP_MULTI(*ap, ifp, inm);
> + joined = (inm != NULL);
> +
> + return (joined);
> +}
>
> void
> in_ifdetach(struct ifnet *ifp)
> Index: netinet/in_var.h
> ===
> RCS file: /cvs/src/sys/netinet/in_var.h,v
> retrieving revision 1.37
> diff -u -p -r1.37 in_var.h
> --- netinet/in_var.h 3 Dec 2015 21:57:59 - 1.37
> +++ netinet/in_var.h 15 Jan 2016 10:34:02 -
> @@ -154,6 +154,7 @@ int in_ifinit(struct ifnet *,
> struct in_ifaddr *, struct sockaddr_in *, int);
> struct in_multi *in_addmulti(struct in_addr *, struct ifnet *);
> void in_delmulti(struct in_multi *);
> +int in_hasmulti(struct in_addr *, struct ifnet *);
> void in_ifscrub(struct ifnet *, struct in_ifaddr *);
> int in_control(struct socket *, u_long, caddr_t, struct ifnet *);
> void in_prefixlen2mask(struct in_addr *, int);
> Index: netinet/ip_carp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.285
> diff -u -p -r1.285 ip_carp.c
> --- netinet/ip_carp.c 12 Jan 2016 09:22:01 - 1.285
> +++ netinet/ip_carp.c 15 Jan 2016 10:47:19 -
> @@ -1809,17 +1809,13 @@ carp_addr_updated(void *v)
>
> /* We received address changes from if_addrhooks callback */
> if (new_naddrs != sc->sc_naddrs || new_naddrs6 != sc->sc_naddrs6) {
> - struct in_addr mc_addr;
> - struct in_multi *inm;
>
> sc->sc_naddrs = new_naddrs;
> sc->sc_naddrs6 = new_naddrs6;
>
> /* Re-establish multicast membership removed by in_control */
> if (IN_MULTICAST(sc->sc_peer.s_addr)) {
> - mc_addr.s_addr = sc->sc_peer.s_addr;
> - IN_LOOKUP_MULTI(mc_addr, >sc_if, inm);
> - if (inm == NULL) {
> + if (!in_hasmulti(>sc_peer, >sc_if)) {
> struct in_multi **imm =
> sc->sc_imo.imo_membership;
> u_int16_t maxmem =
> Index: netinet/ip_input.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.265
> diff -u -p -r1.265 ip_input.c
> --- netinet/ip_input.c3 Dec 2015 21:11:53 - 1.265
> +++ netinet/ip_input.c15 Jan 2016 10:34:30 -
> @@ -346,8 +346,6 @@ ipv4_input(struct mbuf *m)
> }
>
> if (IN_MULTICAST(ip->ip_dst.s_addr)) {
> - struct in_multi *inm;
> -
> /*
>* Make sure M_MCAST is set. It should theoretically
>* already be there, but let's play safe because upper
> @@ -402,8 +400,7 @@ ipv4_input(struct mbuf *m)
>* See if we belong to the destination multicast group on the
>* arrival interface.
>*/
> - IN_LOOKUP_MULTI(ip->ip_dst, ifp, inm);
> - if (inm == NULL) {
> + if (!in_hasmulti(>ip_dst, ifp)) {
> ipstat.ips_notmember++;
> if (!IN_LOCAL_GROUP(ip->ip_dst.s_addr))
> ipstat.ips_cantforward++;
> Index: netinet/ip_output.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.316
> diff -u -p -r1.316 ip_output.c
> --- netinet/ip_output.c 13 Jan 2016 09:38:36 - 1.316
> +++ netinet/ip_output.c 15 Jan 2016 10:43:47 -
> @@ -241,7 +241,6 @@ reroute:
>
> if (IN_MULTICAST(ip->ip_dst.s_addr) ||
> (ip->ip_dst.s_addr == INADDR_BROADCAST)) {
> - struct in_multi *inm;
>
>
in{,6}_hasmulti() for MP
One of the checks missing to have an unlocked forwarding path is related
to multicast. We must ensure that the list of multicast groups attached
to an ifp is not modified when the CPU processing a packet is traversing
it.
In order to prepare for such change the diff below introduces a new
function to abstract the list traversal.
It is a simple refactoring but multiples reviews are more than welcome
at this stage of the release cycle.
ok?
Index: netinet/in.c
===
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.125
diff -u -p -r1.125 in.c
--- netinet/in.c3 Dec 2015 21:57:59 - 1.125
+++ netinet/in.c15 Jan 2016 10:33:36 -
@@ -880,6 +880,21 @@ in_delmulti(struct in_multi *inm)
}
}
+/*
+ * Return 1 if the multicast group represented by ``ap'' has been
+ * joined by interface ``ifp'', 0 otherwise.
+ */
+int
+in_hasmulti(struct in_addr *ap, struct ifnet *ifp)
+{
+ struct in_multi *inm;
+ int joined;
+
+ IN_LOOKUP_MULTI(*ap, ifp, inm);
+ joined = (inm != NULL);
+
+ return (joined);
+}
void
in_ifdetach(struct ifnet *ifp)
Index: netinet/in_var.h
===
RCS file: /cvs/src/sys/netinet/in_var.h,v
retrieving revision 1.37
diff -u -p -r1.37 in_var.h
--- netinet/in_var.h3 Dec 2015 21:57:59 - 1.37
+++ netinet/in_var.h15 Jan 2016 10:34:02 -
@@ -154,6 +154,7 @@ int in_ifinit(struct ifnet *,
struct in_ifaddr *, struct sockaddr_in *, int);
struct in_multi *in_addmulti(struct in_addr *, struct ifnet *);
void in_delmulti(struct in_multi *);
+intin_hasmulti(struct in_addr *, struct ifnet *);
void in_ifscrub(struct ifnet *, struct in_ifaddr *);
intin_control(struct socket *, u_long, caddr_t, struct ifnet *);
void in_prefixlen2mask(struct in_addr *, int);
Index: netinet/ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.285
diff -u -p -r1.285 ip_carp.c
--- netinet/ip_carp.c 12 Jan 2016 09:22:01 - 1.285
+++ netinet/ip_carp.c 15 Jan 2016 10:47:19 -
@@ -1809,17 +1809,13 @@ carp_addr_updated(void *v)
/* We received address changes from if_addrhooks callback */
if (new_naddrs != sc->sc_naddrs || new_naddrs6 != sc->sc_naddrs6) {
- struct in_addr mc_addr;
- struct in_multi *inm;
sc->sc_naddrs = new_naddrs;
sc->sc_naddrs6 = new_naddrs6;
/* Re-establish multicast membership removed by in_control */
if (IN_MULTICAST(sc->sc_peer.s_addr)) {
- mc_addr.s_addr = sc->sc_peer.s_addr;
- IN_LOOKUP_MULTI(mc_addr, >sc_if, inm);
- if (inm == NULL) {
+ if (!in_hasmulti(>sc_peer, >sc_if)) {
struct in_multi **imm =
sc->sc_imo.imo_membership;
u_int16_t maxmem =
Index: netinet/ip_input.c
===
RCS file: /cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.265
diff -u -p -r1.265 ip_input.c
--- netinet/ip_input.c 3 Dec 2015 21:11:53 - 1.265
+++ netinet/ip_input.c 15 Jan 2016 10:34:30 -
@@ -346,8 +346,6 @@ ipv4_input(struct mbuf *m)
}
if (IN_MULTICAST(ip->ip_dst.s_addr)) {
- struct in_multi *inm;
-
/*
* Make sure M_MCAST is set. It should theoretically
* already be there, but let's play safe because upper
@@ -402,8 +400,7 @@ ipv4_input(struct mbuf *m)
* See if we belong to the destination multicast group on the
* arrival interface.
*/
- IN_LOOKUP_MULTI(ip->ip_dst, ifp, inm);
- if (inm == NULL) {
+ if (!in_hasmulti(>ip_dst, ifp)) {
ipstat.ips_notmember++;
if (!IN_LOCAL_GROUP(ip->ip_dst.s_addr))
ipstat.ips_cantforward++;
Index: netinet/ip_output.c
===
RCS file: /cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.316
diff -u -p -r1.316 ip_output.c
--- netinet/ip_output.c 13 Jan 2016 09:38:36 - 1.316
+++ netinet/ip_output.c 15 Jan 2016 10:43:47 -
@@ -241,7 +241,6 @@ reroute:
if (IN_MULTICAST(ip->ip_dst.s_addr) ||
(ip->ip_dst.s_addr == INADDR_BROADCAST)) {
- struct in_multi *inm;
m->m_flags |= (ip->ip_dst.s_addr == INADDR_BROADCAST) ?
M_BCAST : M_MCAST;
@@ -295,9 +294,8 @@ reroute:
ip->ip_src = ia->ia_addr.sin_addr;
}
- IN_LOOKUP_MULTI(ip->ip_dst,
