m_get_rcvif and symmetry

[Maxime Villard]

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.

Thanks,
Maxime

[1] https://nxr.netbsd.org/xref/src/sys/sys/mbuf.h#1014

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Ryota Ozaki]

Post by Maxime Villard
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.

Thanks,
ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Ryota Ozaki]

Post by Ryota Ozaki

Post by Maxime Villard
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

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[J. Lewis Muir]

Post by Ryota Ozaki
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

s/return/returned/

Post by Ryota Ozaki
+ * m_put_rcvif (calling it is safe). The caller cannot block or sleep during

s/calling it is safe/although calling it is safe/
s/during/while/

Post by Ryota Ozaki
+ * using rcvif. Insofar as the constraint is satisfied, the API ensures a got

s/Insofar as the constraint is satisfied, t/T/
s/got/returned/

Post by Ryota Ozaki
+ * rcvif isn't be freed until m_put_rcvif is called.

s/be//

Regards,

Lewis

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Ryota Ozaki]

Post by J. Lewis Muir

Post by Ryota Ozaki
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

s/return/returned/

Post by Ryota Ozaki
+ * m_put_rcvif (calling it is safe). The caller cannot block or sleep during

s/calling it is safe/although calling it is safe/
s/during/while/

Post by Ryota Ozaki
+ * using rcvif. Insofar as the constraint is satisfied, the API ensures a got

s/Insofar as the constraint is satisfied, t/T/
s/got/returned/

Post by Ryota Ozaki
+ * rcvif isn't be freed until m_put_rcvif is called.

s/be//

Thanks! I revised the comment.

ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de