[Click] Linux 2.6.24 and later

[Joonwoo Park]

Hello Eddie,

Eventually!  Thank you.

On Wed, Oct 22, 2008 at 12:49 PM, Eddie Kohler <kohler at cs.ucla.edu> wrote:
> Hi all,
>
> I've gone over and entered versions of most of Joonwoo and Adam's Click
> patch into mainline.  I've attached the remaining portions of the patch.
>  Adam, Joonwoo, can you please explain the parts of this patch to me?  The
> rtnl_lock() for example; is that required with *every* version of Linux [and
> we just got it wrong], or is it something new for 2.6.24?  Similarly for
> commenting out netif_wake_queue() in todevice?  Similarly for KBUILD_CFLAGS
> in the Makefile, and lock() and unlock() in the skbmgr?

IIRC, I did all of left patches.  I've placed my comments.

>
> Progress...
> Eddie
>
>
> diff --git a/elements/linuxmodule/anydevice.cc
> b/elements/linuxmodule/anydevice.cc
> index c19ae22..8633be1 100644
> --- a/elements/linuxmodule/anydevice.cc
> +++ b/elements/linuxmodule/anydevice.cc
> @@ -32,6 +32,7 @@ CLICK_CXX_PROTECT
>  #include <linux/if_arp.h>
>  #endif
>  #include <linux/smp_lock.h>
> +#include <linux/rtnetlink.h>
>  CLICK_CXX_UNPROTECT
>  #include <click/cxxunprotect.h>
>
> @@ -107,8 +108,11 @@ AnyDevice::find_device(AnyDeviceMap *adm, ErrorHandler
> *errh)
>        _dev = 0;
>     }
>
> -    if (_dev && _promisc)
> +    if (_dev && _promisc) {
> +       rtnl_lock();
>        dev_set_promiscuity(_dev, 1);
> +       rtnl_unlock();
> +    }
>  #if HAVE_NET_ENABLE_TIMESTAMP
>     if (_dev && _timestamp)
>        net_enable_timestamp();
> @@ -143,8 +147,11 @@ AnyDevice::set_device(net_device *dev, AnyDeviceMap
> *adm, bool locked)
>            click_chatter("%s: device '%s' went down", declaration().c_str(),
> _devname.c_str());
>     }
>
> -    if (_dev && _promisc)
> +    if (_dev && _promisc) {
> +       rtnl_lock();
>        dev_set_promiscuity(_dev, -1);
> +       rtnl_unlock();
> +    }
>  #if HAVE_NET_ENABLE_TIMESTAMP
>     if (_dev && _timestamp)
>        net_disable_timestamp();
> @@ -160,8 +167,11 @@ AnyDevice::set_device(net_device *dev, AnyDeviceMap
> *adm, bool locked)
>     if (adm)
>        adm->insert(this, locked);
>
> -    if (_dev && _promisc)
> +    if (_dev && _promisc) {
> +       rtnl_lock();
>        dev_set_promiscuity(_dev, 1);
> +       rtnl_unlock();
> +    }
>  #if HAVE_NET_ENABLE_TIMESTAMP
>     if (_dev && _timestamp)
>        net_enable_timestamp();
> @@ -180,8 +190,11 @@ AnyDevice::set_device(net_device *dev, AnyDeviceMap
> *adm, bool locked)
>  void
>  AnyDevice::clear_device(AnyDeviceMap *adm)
>  {
> -    if (_dev && _promisc)
> +    if (_dev && _promisc) {
> +       rtnl_lock();
>        dev_set_promiscuity(_dev, -1);
> +       rtnl_unlock();
> +    }
>  #if HAVE_NET_ENABLE_TIMESTAMP
>     if (_dev && _timestamp)
>        net_disable_timestamp();

Since the version of linux 2.6.22, dev_set_promiscuity() calls
ASSERT_RTNL() so I fixed anydevice to lock before calling it.
The commit of linux is 24023451c8df726692e2f52288a20870d13b501f.

> diff --git a/elements/linuxmodule/todevice.cc
> b/elements/linuxmodule/todevice.cc
> index 0a2eb7c..9029401 100644
> --- a/elements/linuxmodule/todevice.cc
> +++ b/elements/linuxmodule/todevice.cc
> @@ -303,7 +303,7 @@ ToDevice::run_task(Task *)
>     // to call qdisc_restart() ourselves, outside of net_bh().
>     if (is_polling && !busy && _dev->qdisc->q.qlen) {
>        _dev->tx_eob(_dev);
> -       netif_wake_queue(_dev);
> +       //netif_wake_queue(_dev);
>     }
>  #endif
>

I'm sorry, I cannot remember correctly.  Please leave it and I'll let
you update if I figure out.
This patch is very old :(

> diff --git a/linuxmodule/Makefile.in b/linuxmodule/Makefile.in
> index 33a8ed3..c1d55e3 100644
> --- a/linuxmodule/Makefile.in
> +++ b/linuxmodule/Makefile.in
> @@ -84,9 +84,11 @@ CLICK_ELEM2MAKE = $(top_builddir)/click-buildtool
> elem2make --linux26
>  CLICKCC = @KERNEL_CC@
>  CLICKCXX = @KERNEL_CXX@
>
> -LINUXCFLAGS = $(shell echo "$(CPPFLAGS) $(CFLAGS) $(CFLAGS_MODULE)" | sed \
> +LINUXCFLAGS = $(shell echo "$(CPPFLAGS) $(CFLAGS) $(CFLAGS_MODULE)" \
> +       "$(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) " | sed \
>        -e s,-fno-unit-at-a-time,, -e s,-Wstrict-prototypes,, \
>        -e s,-Wdeclaration-after-statement,, \
> +       -e s,-Werror-implicit-function-declaration,, \
>        -e s,-Wno-pointer-sign,, -e s,-fno-common,,)
>  CLICKCPPFLAGS = @CPPFLAGS@ -DCLICK_LINUXMODULE
>  CLICKCFLAGS = @CFLAGS_NDEBUG@

Linux's makefile had changed to use from CPPFLAGS and CFLAGS to
KBUILD_CPPFLAGS and KBUILD_CFLAGS.
I've left CPPFLAGS and CFLAGS is for 2.6.19.
It also has that new flag (,-Werror-implicit-function-declaration), so
I included it into click linuxmodule's makefile as well.  However I
guess it can be removed.

> diff --git a/linuxmodule/skbmgr.cc b/linuxmodule/skbmgr.cc
> index 3a55dae..45c27a7 100644
> --- a/linuxmodule/skbmgr.cc
> +++ b/linuxmodule/skbmgr.cc
> @@ -339,6 +346,7 @@ RecycledSkbPool::allocate(unsigned headroom, unsigned
> size, int want, int *store
>   }
>
>   size = size_to_higher_bucket_size(headroom + size);
> +  lock();
>   while (got < want) {
>     struct sk_buff *skb = alloc_skb(size, GFP_ATOMIC);
>  #if DEBUG_SKBMGR
> @@ -353,6 +361,7 @@ RecycledSkbPool::allocate(unsigned headroom, unsigned
> size, int want, int *store
>     prev = &skb->next;
>     got++;
>   }
> +  unlock();
>
>   *prev = 0;
>   *store_got = got;
>
>

Also, I'm not sure this for now.
It looks like related to the skbmgr multi-threading bug.
For now please leave it alone.  I'll let you know if I have any
problem something that patch is needed.

Thank you!!!
Joonwoo