Re: vlan(4) mpsafe

[Shoichi Yamaguchi <s-yamaguchi%iij.ad.jp@localhost>]

Hi,

Thank you for comments.

I modify and rebase the patch.
Here is the updated patch:

https://gist.githubusercontent.com/s-ymgch228/02bb360cfd4e95eb1f64a66c28b30ab2/raw/1b9c8408da88a205ee1b09482125dcf49d5a762e/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?

I added the comments about locking notes and locking order into
if_vlanvar.h

>     @@ -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.

No, it isn't. All splnet can be eliminated because all data is protected
by locks newly introduced. In the updated patch, they are eliminated.

>     +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.

I didn't know such a restriction. I moved kmem_* before mutex_enter() or
after mutex_exit().

>     @@ -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!

I selected (c) to fix this part in the updated patch.


Thanks,

--
Internet Initiative Japan Inc.

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

Shoichi YAMAGUCHI <s-yamaguchi%iij.ad.jp@localhost>