Re: vlan(4) mpsafe

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Fri, 26 May 2017 18:27:16 +0900
> From: Shoichi Yamaguchi <s-yamaguchi%iij.ad.jp@localhost>
> 
> Hi, I implement vlan(4) MP-ify code.

Cool, thanks for working on this!

> Here is a patch:
> https://gist.githubusercontent.com/s-ymgch228/02bb360cfd4e95eb1f64a66c28b30ab2/raw/97221fcc103a796020ea3c48f0ffdbb7bc845d38/vlan_mpsafe.patch

I don't have time for a detailed review right now, but here are some
quick comments from a cursory glance.  First, this change introduces
several new locks -- struct ifvlan::ifv_lock, ifv_list.lock, and
ifv_hash.lock.  Can you write down a lock order for them?

It looks like ifv_list.lock ---> struct ifvlan::ifv_lock is one
ordering relation, based on vlan_ifdetach, but I'm not sure about the
others.

   @@ -115,6 +122,21 @@ __KERNEL_RCSID(0, "$NetBSD: if_vlan.c,v 1.96 2017/03/15 09:51:08 ozaki-r Exp $")

    #include "ioconf.h"

   +#ifdef NET_MPSAFE
   +#define VLAN_MPSAFE            1
   +#define DECLARE_LOCK_VARIABLE
   +#define ACQUIRE_GLOBAL_LOCKS() do { } while (0)
   +#define RELEASE_GLOBAL_LOCKS() do { } while (0)
   +#else
   +#define DECLARE_LOCK_VARIABLE  int __s
   +#define ACQUIRE_GLOBAL_LOCKS() do {                    \
   +                                       __s = splnet(); \
   +                               } while (0)
   +#define RELEASE_GLOBAL_LOCKS() do {                    \
   +                                       splx(__s);      \
   +                               } while (0)
   +#endif

Is this necessary?  Can we just eliminate the splnet altogether yet?
The name `global locks' is confusing -- splnet only blocks interrupts
on the current CPU.

   +vlan_config(struct ifvlan *ifv, struct ifnet *p, uint16_t tag)
    {
   ...
   +       mutex_enter(&ifv->ifv_lock);
   ...
   +       nmib = kmem_alloc(sizeof(*nmib), KM_SLEEP);
   +       *nmib = *omib;

Using kmem_* under a lock is not generally allowed.  You should
preallocate the structure, and then acquire the lock.  In the error
branch this may require you to free it immediately, but that's OK.

   @@ -370,33 +484,67 @@ vlan_config(struct ifvlan *ifv, struct ifnet *p)
   ...
   +       vlan_linkmib_update(ifv, nmib);
   +       nmib = NULL;
   +       kmem_free(omib, sizeof(*omib));
   +
   +done:
   +       if (nmib) {
   +               psref_target_destroy(&nmib->ifvm_psref, ifvm_psref_class);
   +               kmem_free(nmib, sizeof(*nmib));
   +       }
   +       mutex_exit(&ifv->ifv_lock);

Similarly, kmem_free isn't allowed under a lock either.  Release the
lock first; then psref_target_destroy and kmem_free.

   +vlan_unconfig_locked(struct ifnet *ifp)
   +{
   ...
   +       KASSERT(mutex_owned(&ifv->ifv_lock));
   ...
   +       nmib = kmem_alloc(sizeof(*nmib), KM_SLEEP);

Same here.  This may require a little restructuring -- e.g., maybe you
need to pass a preallocated struct ifvlan_linkmib as another parameter
to vlan_unconfig_locked.

   @@ -370,33 +484,67 @@ vlan_config(struct ifvlan *ifv, struct ifnet *p)
            * to participate in bridges of that type.
            */
           ifv->ifv_if.if_type = p->if_type;
   +       membar_producer();

   -       return (0);
   +       idx = tag_hash_func(tag, ifv_hash.mask);
   +
   +       mutex_enter(&ifv_hash.lock);
   +       PSLIST_WRITER_INSERT_HEAD(&ifv_hash.lists[idx], ifv, ifv_hash);
   +       mutex_exit(&ifv_hash.lock);
   +
   +       vlan_linkmib_update(ifv, nmib);
   +       nmib = NULL;
   +       kmem_free(omib, sizeof(*omib));

This change is a little confusing.  I think you added the
membar_producer here so that the content of nmib would be published
before vlan_linkmib_update publishes the nmib pointer with
ifv->ifv_mib = nmib.  In this case, the membar_producer is not
*strictly* necessary because PSLIST_WRITER_INSERT_HEAD already issues
one.

But membars should generally come in pairs -- pretty much every
membar_(datadep_)consumer should be paired with a corresponding
membar_producer -- so it's best to make it very clear which ones match
up.  So I would suggest that you either

(a) add a comment saying that this membar_producer corresponds to the
membar_datadep_consumer issued after reading ifv->ifv_mib; or

(b) omit the membar_producer, and add a comment explaining that the
membar_producer implied by PSLIST_WRITER_INSERT_HEAD works to match
the membar_datadep_consumer; or

(c) move the explicit membar_producer into vlan_linkmib_update.

It took me about three readings to realize what was going on here, so
the added clarity of any one of these options will make it easier to
audit!

   @@ -419,21 +567,145 @@ vlan_unconfig(struct ifnet *ifp)
    #endif
           }

   -       ifv->ifv_p = NULL;
   +       nmib->ifvm_p = NULL;
           ifv->ifv_if.if_mtu = 0;
           ifv->ifv_flags = 0;
   +       membar_producer();
   +
   +       mutex_enter(&ifv_hash.lock);
   +       PSLIST_WRITER_REMOVE(ifv, ifv_hash);
   +       pserialize_perform(vlan_psz);
   +       mutex_exit(&ifv_hash.lock);
   +
   +       vlan_linkmib_update(ifv, nmib);
   +       kmem_free(omib, sizeof(*omib));

Same issue here -- pserialize_perform effectively issues a
membar_producer (and much more).