Re: mbuf initialization macros

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

Thank you for comments.

On 2016/04/15 17:44, Robert Elz wrote:
>     Date:        Fri, 15 Apr 2016 11:42:22 +0900
>     From:        Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>
>     Message-ID:  <5710550E.4010406%iij.ad.jp@localhost>
> 
>   | Could you comment this patch?
> 
> I would make them static inline functions, rather than macros, so you
> don't need to use "implementation" var names (not that it matters as
> a macro arg) and avoid problems should someone, sometime later, use a
> non-constant arg (m++ or something).

Fair enough. I use static inline function.

> I would also (after an audit of the uses) add any fields that are
> commonly set to something other than NULL/0/etc in the init, and make
> those params to the function - if the example you showed is typical,
> then for M_HDR_INIT() (which as a func would probably be m_hdr_init())
> I'd add a type param (and move the mowner_init() call into the func,)
> as I would expect that frequently code that does things this way is
> likely to want a type that is not MT_DATA.

I see. I add a type param and move the monwer_init().

> But only for those values are frequently set to something specific in the
> current places where you would (eventually) replace inline code by a
> macro/static_inline_func call.   (Doesn't need to be all cases, but a param
> that everyone has to set, just because one place wants the field different
> than default, would be annoying).

As all of mbuf stack allocating functions set the following fields,
I add these fields to params.
    - m_next
    - m_data
    - m_len

Some of the functions set m_flags and m_nextpkt, however m_flags is
almost set 0 and m_nextpkt is always set NULL. So I don't add these
fields.

> For M_PKTHDR_INIT() I'd add data & flags params to set m_data and m_flags.
> That is: make the macro/func turn the mbuf into a pkhhdr, rather than just
> init the pkthdr fields after that has already happened.

I see. I set m_data and m_flags in m_pkthdr_init().

> If any of the other m_pkthdr fields are often set to different than
> the default values (most likely would probably be rcvif) make that a
> param too - just like (probably) "type" for M_HDR_INIT().

Hmm, in the mbuf stack allocating functions, only dp8390_ipkdb_send()
@sys/dev/ic/dp8390.c sets M_PKTHDR to m_flags. The function doesn't
set m_pkthdr fields explicitly. Furthermore, other M_PKTHDR mbuf using
functions use MGETDHR() or m_gethdr(). So, I think m_pkthdr_init()
don't need to add params except for mbuf.

I reflect above comments. Here is updated patch.
====================
diff --git a/sys/kern/uipc_mbuf.c b/sys/kern/uipc_mbuf.c
index f2056da..b48c599 100644
--- a/sys/kern/uipc_mbuf.c
+++ b/sys/kern/uipc_mbuf.c
@@ -583,14 +583,8 @@ m_get(int nowait, int type)
                return NULL;
 
        mbstat_type_add(type, 1);
-       mowner_init(m, type);
-       m->m_ext_ref = m;
-       m->m_type = type;
-       m->m_len = 0;
-       m->m_next = NULL;
-       m->m_nextpkt = NULL;
-       m->m_data = m->m_dat;
-       m->m_flags = 0;
+
+       m_hdr_init(m, type, NULL, 0, m->m_dat);
 
        return m;
 }
@@ -604,13 +598,7 @@ m_gethdr(int nowait, int type)
        if (m == NULL)
                return NULL;
 
-       m->m_data = m->m_pktdat;
-       m->m_flags = M_PKTHDR;
-       m->m_pkthdr.rcvif = NULL;
-       m->m_pkthdr.len = 0;
-       m->m_pkthdr.csum_flags = 0;
-       m->m_pkthdr.csum_data = 0;
-       SLIST_INIT(&m->m_pkthdr.tags);
+       m_pkthdr_init(m);
 
        return m;
 }
diff --git a/sys/sys/mbuf.h b/sys/sys/mbuf.h
index 75f4d64..127933c 100644
--- a/sys/sys/mbuf.h
+++ b/sys/sys/mbuf.h
@@ -867,6 +867,10 @@ int        m_append(struct mbuf *, int, const void *);
 /* Inline routines. */
 static __inline u_int m_length(const struct mbuf *) __unused;
 
+static __inline void m_hdr_init(struct mbuf *, short, struct mbuf *,
+                               char *, int);
+static __inline void m_pkthdr_init(struct mbuf *);
+
 /* Statistics */
 void mbstat_type_add(int, int);
 
@@ -934,6 +938,38 @@ m_length(const struct mbuf *m)
        return pktlen;
 }
 
+static __inline void
+m_hdr_init(struct mbuf *m, short type, struct mbuf *next, char *data, int len)
+{
+
+       KASSERT(m != NULL);
+
+       mowner_init(m, type);
+       m->m_ext_ref = m; /* default */
+       m->m_type = type;
+       m->m_len = len;
+       m->m_next = next;
+       m->m_nextpkt = NULL; /* default */
+       m->m_data = data;
+       m->m_flags = 0; /* default */
+}
+
+static __inline void
+m_pkthdr_init(struct mbuf *m)
+{
+
+       KASSERT(m != NULL);
+
+       m->m_data = m->m_pktdat;
+       m->m_flags = M_PKTHDR;
+
+       m->m_pkthdr.rcvif = NULL;
+       m->m_pkthdr.len = 0;
+       m->m_pkthdr.csum_flags = 0;
+       m->m_pkthdr.csum_data = 0;
+       SLIST_INIT(&m->m_pkthdr.tags);
+}
+
 void m_print(const struct mbuf *, const char *, void (*)(const char *, ...)
     __printflike(1, 2));
 
====================

Could you comment this patch?


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>