Date: Mon, 10 Jul 2006 13:47:23 -0700 From: Atanas <atanas@asd.aplus.net> To: pyunyh@gmail.com Cc: Peter Jeremy <peterjeremy@optushome.com.au>, Robert Watson <rwatson@freebsd.org>, Michael Vince <mv@thebeastie.org>, freebsd-stable@freebsd.org, User Freebsd <freebsd@hub.org> Subject: Re: em device hangs on ifconfig alias ... Message-ID: <44B2BCDB.7050403@asd.aplus.net> In-Reply-To: <20060708033254.GB87930@cdnetworks.co.kr> References: <44AC6793.2070608@asd.aplus.net> <20060706021444.GA76865@cdnetworks.co.kr> <44AD7297.7080605@asd.aplus.net> <20060707010341.GD82406@cdnetworks.co.kr> <44ADC2ED.4070904@asd.aplus.net> <20060707040838.GE82406@cdnetworks.co.kr> <20060707151640.D51390@fledge.watson.org> <44AEB0CB.5060102@asd.aplus.net> <20060707181750.O1171@ganymede.hub.org> <20060707223609.N60542@fledge.watson.org> <20060708033254.GB87930@cdnetworks.co.kr>
next in thread | previous in thread | raw e-mail | index | archive | help
Pyun YongHyeon said the following on 7/7/06 8:32 PM:
> On Fri, Jul 07, 2006 at 10:38:01PM +0100, Robert Watson wrote:
> >
> > Yes -- basically, there are two problems:
> >
> > (1) A little problem, in which an arp announcement is sent before the link
> > has
> > settled after reset.
> >
> > (2) A big problem, in which the interface is gratuitously recent requiring
> > long settling times.
> >
> > I'd really like to see a fix to the second of these problems (not resetting
> > when an IP is added or removed, resulting in link renegotiation); the first
> > one I'm less concerned about, although it would make some amount of sense
> > to do an arp announcement when the link goes up.
> >
>
> Ah, I see. Thanks for the insight.
> How about the attached patch?
>
This patch seems to fix both of the issues, or at least this is what I
see now:
- the card no longer gets reset when adding an alias;
- the arp packet gets delivered;
- adding 250 aliases takes less than a second;
I haven't fully tested whether all 250 IP aliases were accessible (I
used non-routable IP addresses), but I suppose so. Also I couldn't
stress the patched driver enough to see whether it performs as expected.
But in overall it looks good. I guess some more testing might be needed
in order to merge the patch into the source tree.
Regards,
Atanas
>
>
> ------------------------------------------------------------------------
>
> Index: if_em.c
> ===================================================================
> RCS file: /pool/ncvs/src/sys/dev/em/if_em.c,v
> retrieving revision 1.116
> diff -u -r1.116 if_em.c
> --- if_em.c 6 Jun 2006 08:03:49 -0000 1.116
> +++ if_em.c 8 Jul 2006 03:30:36 -0000
> @@ -67,6 +67,7 @@
>
> #include <netinet/in_systm.h>
> #include <netinet/in.h>
> +#include <netinet/if_ether.h>
> #include <netinet/ip.h>
> #include <netinet/tcp.h>
> #include <netinet/udp.h>
> @@ -692,6 +693,9 @@
>
> EM_LOCK_ASSERT(sc);
>
> + if ((ifp->if_drv_flags & (IFF_DRV_RUNNING|IFF_DRV_OACTIVE)) !=
> + IFF_DRV_RUNNING)
> + return;
> if (!sc->link_active)
> return;
>
> @@ -745,6 +749,7 @@
> {
> struct em_softc *sc = ifp->if_softc;
> struct ifreq *ifr = (struct ifreq *)data;
> + struct ifaddr *ifa = (struct ifaddr *)data;
> int error = 0;
>
> if (sc->in_detach)
> @@ -752,9 +757,22 @@
>
> switch (command) {
> case SIOCSIFADDR:
> - case SIOCGIFADDR:
> - IOCTL_DEBUGOUT("ioctl rcv'd: SIOCxIFADDR (Get/Set Interface Addr)");
> - ether_ioctl(ifp, command, data);
> + if (ifa->ifa_addr->sa_family == AF_INET) {
> + /*
> + * XXX
> + * Since resetting hardware takes a very long time
> + * we only initialize the hardware only when it is
> + * absolutely required.
> + */
> + ifp->if_flags |= IFF_UP;
> + if (!(ifp->if_drv_flags & IFF_DRV_RUNNING)) {
> + EM_LOCK(sc);
> + em_init_locked(sc);
> + EM_UNLOCK(sc);
> + }
> + arp_ifinit(ifp, ifa);
> + } else
> + error = ether_ioctl(ifp, command, data);
> break;
> case SIOCSIFMTU:
> {
> @@ -802,17 +820,19 @@
> IOCTL_DEBUGOUT("ioctl rcv'd: SIOCSIFFLAGS (Set Interface Flags)");
> EM_LOCK(sc);
> if (ifp->if_flags & IFF_UP) {
> - if (!(ifp->if_drv_flags & IFF_DRV_RUNNING)) {
> + if ((ifp->if_drv_flags & IFF_DRV_RUNNING)) {
> + if ((ifp->if_flags ^ sc->if_flags) &
> + IFF_PROMISC) {
> + em_disable_promisc(sc);
> + em_set_promisc(sc);
> + }
> + } else
> em_init_locked(sc);
> - }
> -
> - em_disable_promisc(sc);
> - em_set_promisc(sc);
> } else {
> - if (ifp->if_drv_flags & IFF_DRV_RUNNING) {
> + if (ifp->if_drv_flags & IFF_DRV_RUNNING)
> em_stop(sc);
> - }
> }
> + sc->if_flags = ifp->if_flags;
> EM_UNLOCK(sc);
> break;
> case SIOCADDMULTI:
> @@ -878,8 +898,8 @@
> break;
> }
> default:
> - IOCTL_DEBUGOUT1("ioctl received: UNKNOWN (0x%x)", (int)command);
> - error = EINVAL;
> + error = ether_ioctl(ifp, command, data);
> + break;
> }
>
> return (error);
> Index: if_em.h
> ===================================================================
> RCS file: /pool/ncvs/src/sys/dev/em/if_em.h,v
> retrieving revision 1.44
> diff -u -r1.44 if_em.h
> --- if_em.h 15 Feb 2006 08:39:50 -0000 1.44
> +++ if_em.h 8 Jul 2006 03:30:43 -0000
> @@ -259,6 +259,7 @@
> struct callout timer;
> struct callout tx_fifo_timer;
> int io_rid;
> + int if_flags;
> struct mtx mtx;
> int em_insert_vlan_header;
> struct task link_task;
>
>
> ------------------------------------------------------------------------
>
> Index: if_em.c
> ===================================================================
> RCS file: /pool/ncvs/src/sys/dev/em/if_em.c,v
> retrieving revision 1.65.2.16
> diff -u -r1.65.2.16 if_em.c
> --- if_em.c 19 May 2006 00:19:57 -0000 1.65.2.16
> +++ if_em.c 8 Jul 2006 03:29:16 -0000
> @@ -657,8 +657,9 @@
>
> mtx_assert(&adapter->mtx, MA_OWNED);
>
> - if (!adapter->link_active)
> - return;
> + if ((ifp->if_drv_flags & (IFF_DRV_RUNNING|IFF_DRV_OACTIVE)) !=
> + IFF_DRV_RUNNING)
> + return;
>
> while (!IFQ_DRV_IS_EMPTY(&ifp->if_snd)) {
>
> @@ -714,15 +715,29 @@
> {
> struct ifreq *ifr = (struct ifreq *) data;
> struct adapter * adapter = ifp->if_softc;
> + struct ifaddr *ifa = (struct ifaddr *)data;
> int error = 0;
>
> if (adapter->in_detach) return(error);
>
> switch (command) {
> case SIOCSIFADDR:
> - case SIOCGIFADDR:
> - IOCTL_DEBUGOUT("ioctl rcv'd: SIOCxIFADDR (Get/Set Interface Addr)");
> - ether_ioctl(ifp, command, data);
> + if (ifa->ifa_addr->sa_family == AF_INET) {
> + /*
> + * XXX
> + * Since resetting hardware takes a very long time
> + * we only initialize the hardware only when it is
> + * absolutely required.
> + */
> + ifp->if_flags |= IFF_UP;
> + if (!(ifp->if_drv_flags & IFF_DRV_RUNNING)) {
> + EM_LOCK(adapter);
> + em_init_locked(adapter);
> + EM_UNLOCK(adapter);
> + }
> + arp_ifinit(ifp, ifa);
> + } else
> + error = ether_ioctl(ifp, command, data);
> break;
> case SIOCSIFMTU:
> {
> @@ -760,12 +775,14 @@
> IOCTL_DEBUGOUT("ioctl rcv'd: SIOCSIFFLAGS (Set Interface Flags)");
> EM_LOCK(adapter);
> if (ifp->if_flags & IFF_UP) {
> - if (!(ifp->if_drv_flags & IFF_DRV_RUNNING)) {
> + if ((ifp->if_drv_flags & IFF_DRV_RUNNING)) {
> + if ((ifp->if_flags ^ adapter->if_flags) &
> + IFF_PROMISC) {
> + em_disable_promisc(adapter);
> + em_set_promisc(adapter);
> + }
> + } else
> em_init_locked(adapter);
> - }
> -
> - em_disable_promisc(adapter);
> - em_set_promisc(adapter);
> } else {
> if (ifp->if_drv_flags & IFF_DRV_RUNNING) {
> em_stop(adapter);
> @@ -835,8 +852,8 @@
> break;
> }
> default:
> - IOCTL_DEBUGOUT1("ioctl received: UNKNOWN (0x%x)", (int)command);
> - error = EINVAL;
> + error = ether_ioctl(ifp, command, data);
> + break;
> }
>
> return(error);
> Index: if_em.h
> ===================================================================
> RCS file: /pool/ncvs/src/sys/dev/em/if_em.h,v
> retrieving revision 1.32.2.2
> diff -u -r1.32.2.2 if_em.h
> --- if_em.h 25 Nov 2005 14:11:59 -0000 1.32.2.2
> +++ if_em.h 8 Jul 2006 03:29:25 -0000
> @@ -65,6 +65,7 @@
>
> #include <netinet/in_systm.h>
> #include <netinet/in.h>
> +#include <netinet/if_ether.h>
> #include <netinet/ip.h>
> #include <netinet/tcp.h>
> #include <netinet/udp.h>
> @@ -331,6 +332,7 @@
> struct callout timer;
> struct callout tx_fifo_timer;
> int io_rid;
> + int if_flags;
> u_int8_t unit;
> struct mtx mtx;
> int em_insert_vlan_header;
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> freebsd-stable@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-stable
> To unsubscribe, send any mail to "freebsd-stable-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?44B2BCDB.7050403>