Hi Martin,
On Sun, Nov 12, 2017 at 03:40:59PM +0100, Martin Pieuchot wrote:
> [...]
> It does, some comments below.
> [...]
Wonderful.
> [...]
> This would be an approximation because it might happen that after
> returning NULL the second pool_get(9) won't sleep. However I think
> it's the way to go because m_get(9) calls that can wait are generally
> not performance critical.
> [...]
I had not considered that the second pool_get might not block at all. On
the other hand `netstat -m` says "requests for memory delayed" for that
counter, so returning immediately on the second try not counting as a
delay is OK, I think.
> [...]
> Please do not expand the splx(). Only the counter need it. Simply put
> the NULL check after the splx().
> [...]
I'm not sure I understand. If I move the NULL check after the splx(),
counters_leave() will already have been called so increasing the counter
value has no effect anymore. The only additional things running under
splnet will be a few assignments, so I think moving the splx() a bit
further down does not hurt too much.
Alternatively, I've attached a slightly different suggestion which
doesn't expand the scope of the splx() but duplicates a bit of code.
Does that make more sense?
--
Gregor
Index: uipc_mbuf.c
===================================================================
RCS file: /home/cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.250
diff -u -p -r1.250 uipc_mbuf.c
--- uipc_mbuf.c 12 Oct 2017 09:14:16 -0000 1.250
+++ uipc_mbuf.c 12 Nov 2017 22:09:00 -0000
@@ -233,11 +233,15 @@ m_get(int nowait, int type)
KDASSERT(type < MT_NTYPES);
m = pool_get(&mbpool, nowait == M_WAIT ? PR_WAITOK : PR_NOWAIT);
- if (m == NULL)
- return (NULL);
s = splnet();
counters = counters_enter(&cr, mbstat);
+ if (m == NULL) {
+ counters[MBSTAT_DROPS]++;
+ counters_leave(&cr, mbstat);
+ splx(s);
+ return NULL;
+ }
counters[type]++;
counters_leave(&cr, mbstat);
splx(s);
@@ -266,11 +270,15 @@ m_gethdr(int nowait, int type)
KDASSERT(type < MT_NTYPES);
m = pool_get(&mbpool, nowait == M_WAIT ? PR_WAITOK : PR_NOWAIT);
- if (m == NULL)
- return (NULL);
s = splnet();
counters = counters_enter(&cr, mbstat);
+ if (m == NULL) {
+ counters[MBSTAT_DROPS]++;
+ counters_leave(&cr, mbstat);
+ splx(s);
+ return NULL;
+ }
counters[type]++;
counters_leave(&cr, mbstat);
splx(s);
@@ -349,7 +357,10 @@ m_clget(struct mbuf *m, int how, u_int p
{
struct mbuf *m0 = NULL;
struct pool *pp;
+ struct counters_ref cr;
+ uint64_t *counters;
caddr_t buf;
+ int s;
pp = m_clpool(pktlen);
#ifdef DIAGNOSTIC
@@ -364,9 +375,16 @@ m_clget(struct mbuf *m, int how, u_int p
m = m0;
}
+
buf = pool_get(pp, how == M_WAIT ? PR_WAITOK : PR_NOWAIT);
+
if (buf == NULL) {
m_freem(m0);
+ s = splnet();
+ counters = counters_enter(&cr, mbstat);
+ counters[MBSTAT_DROPS]++;
+ counters_leave(&cr, mbstat);
+ splx(s);
return (NULL);
}
