Re: m_get_rcvif and symmetry

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Fri, Feb 3, 2017 at 11:54 AM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> On Thu, Feb 2, 2017 at 3:12 AM, Maxime Villard <max%m00nbsd.net@localhost> wrote:
>> Hi,
>> Simply looking at the code, is it normal that m_get_rcvif and m_put_rcvif
>> are not symmetric [1]? The former always calls pserialize_read_enter, but
>> the latter may not call pserialize_read_exit.
>>
>> If it is intentional, please add a comment to clarify.
>
> Hmm... m_put_rcvif is wrong. I don't remember why I coded it like that :-/
> I'll fix it. And some usages of them are wrong, I'll fix them too.

During fixing the usages I remember what I wanted to achieve by the API
and found what I should fix is m_get_rcvif, not m_put_rcvif.

The change is below. By the change, a caller must call m_put_rcvif only if
a returned rcvif is non-NULL. If rcvif is NULL, a caller don't need to call
it while a caller can call it safely. So codes of callers sometimes can be
simplified. And the behavior is the same as other APIs such as
m_get_rcvif_psref/m_put_rcvif_psref and if_get/if_put.

--- a/sys/sys/mbuf.h
+++ b/sys/sys/mbuf.h
@@ -1006,16 +1006,23 @@ void m_print(const struct mbuf *, const char
*, void (*)(const char *, ...)
 /*
  * Get rcvif of a mbuf.
  *
- * The caller must call m_put_rcvif after using rcvif. The caller cannot
- * block or sleep during using rcvif. Insofar as the constraint is satisfied,
- * the API ensures a got rcvif isn't be freed until m_put_rcvif is called.
+ * The caller must call m_put_rcvif after using rcvif if the returned rcvif
+ * isn't NULL. If the return rcvif is NULL, the caller doesn't need to call
+ * m_put_rcvif (calling it is safe). The caller cannot block or sleep during
+ * using rcvif. Insofar as the constraint is satisfied, the API ensures a got
+ * rcvif isn't be freed until m_put_rcvif is called.
  */
 static __inline struct ifnet *
 m_get_rcvif(const struct mbuf *m, int *s)
 {
+       struct ifnet *ifp;

        *s = pserialize_read_enter();
-       return if_byindex(m->m_pkthdr.rcvif_index);
+       ifp = if_byindex(m->m_pkthdr.rcvif_index);
+       if (__predict_false(ifp == NULL))
+               pserialize_read_exit(*s);
+
+       return ifp;
 }


The complete patch is:
  http://www.netbsd.org/~ozaki-r/fix-m_getput_rcvif.diff

Thanks,
  ozaki-r