On 05/04/16(Tue) 09:20, David Gwynne wrote:
> vlan config is now done via the vnet and parent ioctls. the idea
> is config can be set up while the vlan interface is down, and then
> "committed' when it is brought up and running.
>
> vlan up does checks of the vlan state against the target parent,
> applies any config necessary to the parent, and then configures the
> vlan interface to handle packets.
>
> vlan down just goes in reverse.
>
> a side effect of this is we can mark the driver as mpsafe.
>
> tests? ok?
I like it. I still find it very hard to review because your diff mix
functionalities change, style rewrite and typos.
I still strongly believe that we should agree on a general solution for
the srp_upate() sleeping problem before adding more specific locks.
I have some few comments, I'm not sure which part of this diff needs
careful review.
> Index: if_vlan.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_vlan.c,v
> retrieving revision 1.158
> diff -u -p -r1.158 if_vlan.c
> --- if_vlan.c 29 Mar 2016 13:55:42 -0000 1.158
> +++ if_vlan.c 4 Apr 2016 13:00:38 -0000
> @@ -80,23 +80,36 @@
> SRPL_HEAD(, ifvlan) *vlan_tagh, *svlan_tagh;
> struct rwlock vlan_tagh_lk = RWLOCK_INITIALIZER("vlantag");
>
> -int vlan_input(struct ifnet *, struct mbuf *, void *);
> -void vlan_start(struct ifnet *ifp);
> -int vlan_ioctl(struct ifnet *ifp, u_long cmd, caddr_t addr);
> -int vlan_unconfig(struct ifnet *ifp, struct ifnet *newp);
> -int vlan_config(struct ifvlan *, struct ifnet *, u_int16_t);
> -void vlan_vlandev_state(void *);
> void vlanattach(int count);
> -int vlan_set_promisc(struct ifnet *ifp);
> int vlan_clone_create(struct if_clone *, int);
> int vlan_clone_destroy(struct ifnet *);
> -void vlan_ifdetach(void *);
> +
> +int vlan_input(struct ifnet *, struct mbuf *, void *);
> +void vlan_start(struct ifnet *ifp);
> +int vlan_ioctl(struct ifnet *ifp, u_long cmd, caddr_t addr);
> +
> +int vlan_up(struct ifvlan *);
> +int vlan_parent_up(struct ifvlan *, struct ifnet *);
> +int vlan_down(struct ifvlan *);
So *_up() and *_down() are the new *_init() and *_stop()? I find these
name really confusing. I first though but why is vlan_down() called in
vlan_clone_destroy(), I made sure the if layer called if_down() before.
For me up/down are related to the link state change.
> +int vlan_promisc(struct ifvlan *, int);
> +
> +void vlan_ifp0_detach_hook(void *);
^^^^^
Other drivers use the equivalent of vlan_ifdetach() why rename it to
introduce a difference?
> @@ -159,9 +172,12 @@ vlan_clone_create(struct if_clone *ifc,
>
> refcnt_init(&ifv->ifv_refcnt);
>
> + ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST;
> + ifp->if_xflags = IFXF_MPSAFE;
> ifp->if_start = vlan_start;
> ifp->if_ioctl = vlan_ioctl;
> - IFQ_SET_MAXLEN(&ifp->if_snd, 1);
> + ifp->if_hardmtu = ~0;
Other pseudo-drivers use 0xffff instead of ~0.
> + ifp->if_link_state = LINK_STATE_DOWN;
Why do we need to set the link state at all during initialization?
> IFQ_SET_READY(&ifp->if_snd);
> if_attach(ifp);
> ether_ifattach(ifp);
> @@ -191,7 +207,9 @@ vlan_clone_destroy(struct ifnet *ifp)
> {
> struct ifvlan *ifv = ifp->if_softc;
>
> - vlan_unconfig(ifp, NULL);
> + if (ISSET(ifp->if_flags, IFF_RUNNING))
> + vlan_down(ifv);
> +
> ether_ifdetach(ifp);
> if_detach(ifp);
> refcnt_finalize(&ifv->ifv_refcnt, "vlanrefs");
> }
>
> int
> -vlan_config(struct ifvlan *ifv, struct ifnet *ifp0, u_int16_t tag)
> +vlan_parent_up(struct ifvlan *ifv, struct ifnet *ifp0)
> {
> - struct sockaddr_dl *sdl1, *sdl2;
> - SRPL_HEAD(, ifvlan) *tagh, *list;
> - u_int flags;
> -
> - if (ifp0->if_type != IFT_ETHER)
> - return EPROTONOSUPPORT;
> - if (ifp0->if_index == ifv->ifv_p && ifv->ifv_tag == tag) /* noop */
> - return (0);
> + int error;
>
> - /* Remember existing interface flags and reset the interface */
> - flags = ifv->ifv_flags;
> - vlan_unconfig(&ifv->ifv_if, ifp0);
> - ifv->ifv_p = ifp0->if_index;
> - ifv->ifv_if.if_baudrate = ifp0->if_baudrate;
> -
> - if (ifp0->if_capabilities & IFCAP_VLAN_MTU) {
> - ifv->ifv_if.if_mtu = ifp0->if_mtu;
> - ifv->ifv_if.if_hardmtu = ifp0->if_hardmtu;
> - } else {
> - ifv->ifv_if.if_mtu = ifp0->if_mtu - EVL_ENCAPLEN;
> - ifv->ifv_if.if_hardmtu = ifp0->if_hardmtu - EVL_ENCAPLEN;
> + /* Register callback for physical link state changes */
> + ifv->lh_cookie = hook_establish(ifp0->if_linkstatehooks, 1,
> + vlan_link_hook, ifv);
> +
> + /* Register callback if parent wants to unregister */
> + ifv->dh_cookie = hook_establish(ifp0->if_detachhooks, 0,
> + vlan_ifp0_detach_hook, ifv);
> +
> + vlan_multi_apply(ifv, ifp0, SIOCADDMULTI);
> +
> + if (ISSET(ifv->ifv_flags, IFVF_PROMISC)) {
> + error = ifpromisc(ifp0, 1);
> + if (error != 0)
> + goto delmulti;
> }
Why not start by setting the parent in promiscuous if asked to? This
would simplify the error path.
>
> - ifv->ifv_if.if_flags = ifp0->if_flags &
> - (IFF_UP | IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST);
> + if_ih_insert(ifp0, vlan_input, NULL);
>
> - /* Reset promisc mode on the interface and its parent */
> - if (flags & IFVF_PROMISC) {
> - ifv->ifv_if.if_flags |= IFF_PROMISC;
> - vlan_set_promisc(&ifv->ifv_if);
> + return (0);
> +
> +delmulti:
> + vlan_multi_apply(ifv, ifp0, SIOCDELMULTI);
> + hook_disestablish(ifp0->if_detachhooks, ifv->dh_cookie);
> + hook_disestablish(ifp0->if_linkstatehooks, ifv->lh_cookie);
> + return (error);
> +}
> +
> +int
> +vlan_up(struct ifvlan *ifv)
> +{
> + SRPL_HEAD(, ifvlan) *tagh, *list;
> + struct ifnet *ifp = &ifv->ifv_if;
> + struct ifnet *ifp0;
> + int error = 0;
> + u_int hardmtu;
> +
> + KASSERT(!ISSET(ifp->if_flags, IFF_RUNNING));
> +
> + tagh = ifv->ifv_type == ETHERTYPE_QINQ ? svlan_tagh : vlan_tagh;
> + list = &tagh[TAG_HASH(ifv->ifv_tag)];
> +
> + ifp0 = if_get(ifv->ifv_p);
> + if (ifp0 == NULL)
> + return (ENXIO);
> +
> + /* check vlan will work on top of the parent */
> + if (ifp0->if_type != IFT_ETHER) {
> + error = EPROTONOSUPPORT;
> + goto put;
> }
This is already checked in the iocl(2) and tun(4) can no longer change
its sex, so is it really needed?
> + hardmtu = ifp0->if_hardmtu;
> + if (!ISSET(ifp0->if_capabilities, IFCAP_VLAN_MTU))
> + hardmtu -= EVL_ENCAPLEN;
> +
> + if (ifp->if_mtu > hardmtu) {
> + error = ENOBUFS;
> + goto put;
> + }
> +
> + /* parent is fine, let's prepare the ifv to handle packets */
> + ifp->if_hardmtu = hardmtu;
> + SET(ifp->if_flags, ifp0->if_flags & IFF_SIMPLEX);
> + if_setlladdr(ifp, LLADDR(ifp0->if_sadl));
> +
> if (ifv->ifv_type != ETHERTYPE_VLAN) {
> /*
> - * Hardware offload only works with the default VLAN
> + * Hardware offloads only works with the default VLAN
> * ethernet type (0x8100).
> */
> - ifv->ifv_if.if_capabilities = 0;
> - } else if (ifp0->if_capabilities & IFCAP_VLAN_HWTAGGING) {
> + ifp->if_capabilities = 0;
> + } else if (ISSET(ifp0->if_capabilities, IFCAP_VLAN_HWTAGGING)) {
> /*
> * If the parent interface can do hardware-assisted
> * VLAN encapsulation, then propagate its hardware-
> @@ -448,266 +494,424 @@ vlan_config(struct ifvlan *ifv, struct i
> * If the card cannot handle hardware tagging, it cannot
> * possibly compute the correct checksums for tagged packets.
> */
> - ifv->ifv_if.if_capabilities = ifp0->if_capabilities &
> - IFCAP_CSUM_MASK;
> + ifp->if_capabilities = ifp0->if_capabilities & IFCAP_CSUM_MASK;
> }
>
> - /*
> - * Set up our ``Ethernet address'' to reflect the underlying
> - * physical interface's.
> - */
> - sdl1 = ifv->ifv_if.if_sadl;
> - sdl2 = ifp0->if_sadl;
> - sdl1->sdl_type = IFT_ETHER;
> - sdl1->sdl_alen = ETHER_ADDR_LEN;
> - bcopy(LLADDR(sdl2), LLADDR(sdl1), ETHER_ADDR_LEN);
> - bcopy(LLADDR(sdl2), ifv->ifv_ac.ac_enaddr, ETHER_ADDR_LEN);
> -
> - ifv->ifv_tag = tag;
> + /* commit the ifv */
> + error = rw_enter(&vlan_tagh_lk, RW_WRITE | RW_INTR);
> + if (error != 0)
> + goto scrub;
>
> - /* Register callback for physical link state changes */
> - ifv->lh_cookie = hook_establish(ifp0->if_linkstatehooks, 1,
> - vlan_vlandev_state, ifv);
> -
> - /* Register callback if parent wants to unregister */
> - ifv->dh_cookie = hook_establish(ifp0->if_detachhooks, 0,
> - vlan_ifdetach, ifv);
> + error = vlan_inuse_locked(ifv->ifv_type, ifv->ifv_p, ifv->ifv_tag);
> + if (error != 0)
> + goto leave;
>
> - vlan_multi_apply(ifv, ifp0, SIOCADDMULTI);
> + SRPL_INSERT_HEAD_LOCKED(&vlan_tagh_rc, list, ifv, ifv_list);
> + rw_exit(&vlan_tagh_lk);
>
> - vlan_vlandev_state(ifv);
> + /* configure the parent to handle packets for this vlan */
> + error = vlan_parent_up(ifv, ifp0);
> + if (error != 0)
> + goto remove;
> +
> + /* we're running now */
> + SET(ifp->if_flags, IFF_RUNNING);
> + vlan_link_state(ifv, ifp0->if_link_state, ifp0->if_baudrate);
>
> - /* Change input handler of the physical interface. */
> - if_ih_insert(ifp0, vlan_input, NULL);
> + if_put(ifp0);
>
> - tagh = ifv->ifv_type == ETHERTYPE_QINQ ? svlan_tagh : vlan_tagh;
> - list = &tagh[TAG_HASH(tag)];
> + return (0);
>
> - rw_enter_write(&vlan_tagh_lk);
> - SRPL_INSERT_HEAD_LOCKED(&vlan_tagh_rc, list, ifv, ifv_list);
> - rw_exit_write(&vlan_tagh_lk);
> +remove:
> + rw_enter(&vlan_tagh_lk, RW_WRITE);
> + SRPL_REMOVE_LOCKED(&vlan_tagh_rc, list, ifv, ifvlan, ifv_list);
> +leave:
> + rw_exit(&vlan_tagh_lk);
> +scrub:
> + ifp->if_capabilities = 0;
> + if_setlladdr(ifp, etheranyaddr);
> + CLR(ifp->if_flags, IFF_SIMPLEX);
> + ifp->if_hardmtu = ~0;
> +put:
> + if_put(ifp0);
>
> - return (0);
> + return (error);
> }
>
> int
> -vlan_unconfig(struct ifnet *ifp, struct ifnet *newifp0)
> +vlan_down(struct ifvlan *ifv)
> {
> - struct sockaddr_dl *sdl;
> - struct ifvlan *ifv;
> - SRPL_HEAD(, ifvlan) *tagh, *list;
> - struct ifnet *ifp0;
> -
> - ifv = ifp->if_softc;
> - ifp0 = if_get(ifv->ifv_p);
> - if (ifp0 == NULL)
> - goto disconnect;
> -
> - /* Unset promisc mode on the interface and its parent */
> - if (ifv->ifv_flags & IFVF_PROMISC) {
> - ifp->if_flags &= ~IFF_PROMISC;
> - vlan_set_promisc(ifp);
> - }
> + SRPL_HEAD(, ifvlan) *tagh, *list;
> + struct ifnet *ifp = &ifv->ifv_if;
> + struct ifnet *ifp0;
>
> tagh = ifv->ifv_type == ETHERTYPE_QINQ ? svlan_tagh : vlan_tagh;
> list = &tagh[TAG_HASH(ifv->ifv_tag)];
>
> + KASSERT(ISSET(ifp->if_flags, IFF_RUNNING));
> +
> + vlan_link_state(ifv, LINK_STATE_DOWN, 0);
This shouldn't be necessary.
> + CLR(ifp->if_flags, IFF_RUNNING);
> +
> + ifq_barrier(&ifp->if_snd);
> +
> + ifp0 = if_get(ifv->ifv_p);
> + if (ifp0 != NULL) {
> + if_ih_remove(ifp0, vlan_input, NULL);
> + if (ISSET(ifv->ifv_flags, IFVF_PROMISC))
> + ifpromisc(ifp0, 0);
> + vlan_multi_apply(ifv, ifp0, SIOCDELMULTI);
> + hook_disestablish(ifp0->if_detachhooks, ifv->dh_cookie);
> + hook_disestablish(ifp0->if_linkstatehooks, ifv->lh_cookie);
> + }
> + if_put(ifp0);
> +
> rw_enter_write(&vlan_tagh_lk);
> SRPL_REMOVE_LOCKED(&vlan_tagh_rc, list, ifv, ifvlan, ifv_list);
> rw_exit_write(&vlan_tagh_lk);
>
> - /* Restore previous input handler. */
> - if_ih_remove(ifp0, vlan_input, NULL);
> -
> - hook_disestablish(ifp0->if_linkstatehooks, ifv->lh_cookie);
> - hook_disestablish(ifp0->if_detachhooks, ifv->dh_cookie);
> - /* Reset link state */
> - if (newifp0 != NULL) {
> - ifp->if_link_state = LINK_STATE_INVALID;
> - if_link_state_change(ifp);
> - }
> + ifp->if_capabilities = 0;
> + if_setlladdr(ifp, etheranyaddr);
> + CLR(ifp->if_flags, IFF_SIMPLEX);
> + ifp->if_hardmtu = ~0;
>
> - /*
> - * Since the interface is being unconfigured, we need to
> - * empty the list of multicast groups that we may have joined
> - * while we were alive and remove them from the parent's list
> - * as well.
> - */
> - vlan_multi_apply(ifv, ifp0, SIOCDELMULTI);
> + return (0);
> +}
