On Sat, Feb 06, 2021 at 05:58:35PM +0300, Vitaliy Makkoveev wrote:
> I???m not sure it should be atomic. It seems groups require their own
> lock and this lock should be held while we perform if_addgroup() and
> if_delgroup().
I also think that atomic refcounting is not needed here. But it
does no harm as adding interface groups is not performance critical.
Question is if we want to use refcnt_... API even if it does more
than required. Or should we go with a self crafted ++ -- refcounting?
I think the API provides a nicer interface.
bluhm
> However if_creategroup() should set `ifg_refcnt??? to 1
> and carp(4) should not touch groups internals.
>
> >
> > Index: net/if.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
> > retrieving revision 1.626
> > diff -u -p -r1.626 if.c
> > --- net/if.c 1 Feb 2021 07:43:33 -0000 1.626
> > +++ net/if.c 6 Feb 2021 12:16:20 -0000
> > @@ -2601,7 +2601,7 @@ if_creategroup(const char *groupname)
> > return (NULL);
> >
> > strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group));
> > - ifg->ifg_refcnt = 0;
> > + refcnt_init(&ifg->ifg_refcnt);
> > ifg->ifg_carp_demoted = 0;
> > TAILQ_INIT(&ifg->ifg_members);
> > #if NPF > 0
> > @@ -2642,13 +2642,17 @@ if_addgroup(struct ifnet *ifp, const cha
> > if (!strcmp(ifg->ifg_group, groupname))
> > break;
> >
> > - if (ifg == NULL && (ifg = if_creategroup(groupname)) == NULL) {
> > + if (ifg == NULL)
> > + ifg = if_creategroup(groupname);
> > + else
> > + refcnt_take(&ifg->ifg_refcnt);
> > +
> > + if (ifg == NULL) {
> > free(ifgl, M_TEMP, sizeof(*ifgl));
> > free(ifgm, M_TEMP, sizeof(*ifgm));
> > return (ENOMEM);
> > }
> >
> > - ifg->ifg_refcnt++;
> > ifgl->ifgl_group = ifg;
> > ifgm->ifgm_ifp = ifp;
> >
> > @@ -2692,7 +2696,7 @@ if_delgroup(struct ifnet *ifp, const cha
> > pfi_group_change(groupname);
> > #endif
> >
> > - if (--ifgl->ifgl_group->ifg_refcnt == 0) {
> > + if (refcnt_rele(&ifgl->ifgl_group->ifg_refcnt)) {
> > TAILQ_REMOVE(&ifg_head, ifgl->ifgl_group, ifg_next);
> > #if NPF > 0
> > pfi_detach_ifgroup(ifgl->ifgl_group);
> > Index: net/if_var.h
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_var.h,v
> > retrieving revision 1.112
> > diff -u -p -r1.112 if_var.h
> > --- net/if_var.h 29 Jul 2020 12:09:31 -0000 1.112
> > +++ net/if_var.h 6 Feb 2021 12:11:35 -0000
> > @@ -263,7 +263,7 @@ struct ifmaddr {
> >
> > struct ifg_group {
> > char ifg_group[IFNAMSIZ];
> > - u_int ifg_refcnt;
> > + struct refcnt ifg_refcnt;
> > caddr_t ifg_pf_kif;
> > int ifg_carp_demoted;
> > TAILQ_HEAD(, ifg_member) ifg_members;
> > Index: netinet/ip_carp.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_carp.c,v
> > retrieving revision 1.351
> > diff -u -p -r1.351 ip_carp.c
> > --- netinet/ip_carp.c 21 Jan 2021 13:18:07 -0000 1.351
> > +++ netinet/ip_carp.c 6 Feb 2021 12:11:35 -0000
> > @@ -786,10 +786,7 @@ carp_sysctl(int *name, u_int namelen, vo
> > void
> > carpattach(int n)
> > {
> > - struct ifg_group *ifg;
> > -
> > - if ((ifg = if_creategroup("carp")) != NULL)
> > - ifg->ifg_refcnt++; /* keep around even if empty */
> > + if_creategroup("carp"); /* keep around even if empty */
> > if_clone_attach(&carp_cloner);
> > carpcounters = counters_alloc(carps_ncounters);
> > }
> >
>
