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?
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, \
> > >
> >
> >
