> Date: Tue, 27 Jun 2023 17:52:44 +0200
> From: Alexander Bluhm <alexander.bl...@gmx.net>
>
> On Tue, Jun 27, 2023 at 01:55:23PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 27 Jun 2023 11:09:32 +0000
> > > From: Klemens Nanni <k...@openbsd.org>
> > >
> > > On Tue, Jun 27, 2023 at 01:32:37PM +0300, Vitaliy Makkoveev wrote:
> > > > M_TEMP seems unreasonable for interface groups data allocations.
> > >
> > > After claudio pointed out the wrong type, I thought of the same name,
> > > no other malloc(9) type fits.
> > >
> > > FWIW OK kn, but please wait for other to chime in.
> >
> > I don't think the interface groups are likely candidates for memory
> > leaks so having a separate type for them seems to be overkill. But
>
> Leaks are always where you don't expect them. It is important for
> me to exclude areas when you are hunting them.
>
> > indeed there is no other suitable type. Maybe we can turn this into
> > M_IFMISC in the future if we need another network interface related
> > type. But for now this is fine.
>
> M_TEMP is bad, anything else is better. I prefer fine grained
> counters as it simplifies bug hunting.
>
> What is the prefered way between pool and malloc? I thought malloc
> is for dynamic size or historic code, but pool is for fixed size
> elements. Does it make sense to create a pool for every fixed sized
> struct?
I don't think it makes sense to create a pool for things that you only
allocate a handful of items from. But otherwise, yes, use pools for
fixed-size items.
> Here we have struct ifg_list, ifg_group, and ifg_member. Three
> pools feel like overkill. One malloc type for all of them looks
> reasonable.
>
> OK bluhm@
>
> > > > Don't forget to recompile systat(1) and vmstat(8) with new sys/malloc.h.
> > > >
> > > > Index: sys/net/if.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/net/if.c,v
> > > > retrieving revision 1.700
> > > > diff -u -p -r1.700 if.c
> > > > --- sys/net/if.c 12 Jun 2023 21:19:54 -0000 1.700
> > > > +++ sys/net/if.c 27 Jun 2023 10:15:12 -0000
> > > > @@ -2784,7 +2784,7 @@ if_creategroup(const char *groupname)
> > > > {
> > > > struct ifg_group *ifg;
> > > >
> > > > - if ((ifg = malloc(sizeof(*ifg), M_TEMP, M_NOWAIT)) == NULL)
> > > > + if ((ifg = malloc(sizeof(*ifg), M_IFGROUP, M_NOWAIT)) == NULL)
> > > > return (NULL);
> > > >
> > > > strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group));
> > > > @@ -2819,11 +2819,11 @@ if_addgroup(struct ifnet *ifp, const cha
> > > > if (!strcmp(ifgl->ifgl_group->ifg_group, groupname))
> > > > return (EEXIST);
> > > >
> > > > - if ((ifgl = malloc(sizeof(*ifgl), M_TEMP, M_NOWAIT)) == NULL)
> > > > + if ((ifgl = malloc(sizeof(*ifgl), M_IFGROUP, M_NOWAIT)) == NULL)
> > > > return (ENOMEM);
> > > >
> > > > - if ((ifgm = malloc(sizeof(*ifgm), M_TEMP, M_NOWAIT)) == NULL) {
> > > > - free(ifgl, M_TEMP, sizeof(*ifgl));
> > > > + if ((ifgm = malloc(sizeof(*ifgm), M_IFGROUP, M_NOWAIT)) ==
> > > > NULL) {
> > > > + free(ifgl, M_IFGROUP, sizeof(*ifgl));
> > > > return (ENOMEM);
> > > > }
> > > >
> > > > @@ -2834,8 +2834,8 @@ if_addgroup(struct ifnet *ifp, const cha
> > > > if (ifg == NULL) {
> > > > ifg = if_creategroup(groupname);
> > > > if (ifg == NULL) {
> > > > - free(ifgl, M_TEMP, sizeof(*ifgl));
> > > > - free(ifgm, M_TEMP, sizeof(*ifgm));
> > > > + free(ifgl, M_IFGROUP, sizeof(*ifgl));
> > > > + free(ifgm, M_IFGROUP, sizeof(*ifgm));
> > > > return (ENOMEM);
> > > > }
> > > > } else
> > > > @@ -2878,7 +2878,7 @@ if_delgroup(struct ifnet *ifp, const cha
> > > >
> > > > if (ifgm != NULL) {
> > > > TAILQ_REMOVE(&ifgl->ifgl_group->ifg_members, ifgm,
> > > > ifgm_next);
> > > > - free(ifgm, M_TEMP, sizeof(*ifgm));
> > > > + free(ifgm, M_IFGROUP, sizeof(*ifgm));
> > > > }
> > > >
> > > > #if NPF > 0
> > > > @@ -2891,10 +2891,10 @@ if_delgroup(struct ifnet *ifp, const cha
> > > > #if NPF > 0
> > > > pfi_detach_ifgroup(ifgl->ifgl_group);
> > > > #endif
> > > > - free(ifgl->ifgl_group, M_TEMP,
> > > > sizeof(*ifgl->ifgl_group));
> > > > + free(ifgl->ifgl_group, M_IFGROUP,
> > > > sizeof(*ifgl->ifgl_group));
> > > > }
> > > >
> > > > - free(ifgl, M_TEMP, sizeof(*ifgl));
> > > > + free(ifgl, M_IFGROUP, sizeof(*ifgl));
> > > >
> > > > return (0);
> > > > }
> > > > Index: sys/sys/malloc.h
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/sys/malloc.h,v
> > > > retrieving revision 1.122
> > > > diff -u -p -r1.122 malloc.h
> > > > --- sys/sys/malloc.h 3 Feb 2022 17:18:22 -0000 1.122
> > > > +++ sys/sys/malloc.h 27 Jun 2023 10:15:13 -0000
> > > > @@ -72,7 +72,7 @@
> > > > /* 7 - free */
> > > > /* 8 - free */
> > > > #define M_IFADDR 9 /* interface address */
> > > > -/* 10 - free */
> > > > +#define M_IFGROUP 10 /* interface group */
> > > > #define M_SYSCTL 11 /* sysctl buffers (persistent
> > > > storage) */
> > > > #define M_COUNTERS 12 /* per CPU counters */
> > > > /* 13 - free */
> > > > @@ -190,7 +190,7 @@
> > > > NULL, \
> > > > NULL, \
> > > > "ifaddr", /* 9 M_IFADDR */ \
> > > > - NULL, \
> > > > + "ifgroup", /* 10 M_IFGROUP */ \
> > > > "sysctl", /* 11 M_SYSCTL */ \
> > > > "counters", /* 12 M_COUNTERS */ \
> > > > NULL, \
> > > >
> > >
> > >
>
