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);
        }

Reply via email to