[PATCH v2 6/6] nl80211: Use global netlink objects.

[Ben Greear]

On 10/20/2011 04:23 PM, Jouni Malinen wrote:
> On Thu, Oct 20, 2011 at 01:21:24PM -0700, greearb at candelatech.com wrote:
>> Netlink sockets can be shared among all driver instances,
>> saving lots of sockets, spurious log messages, memory,
>> and CPU usage when using multiple interfaces in a single
>> process.
>
>> -#define DRVNL nl80211
>> -#define DRVPREQ nl_handle_preq
>> +#define DRVNL global->nl80211
>> +#define DRVPREQ global->nl_handle_preq
>
> Uh... Why would those macros be used? Would be just much cleaner to move
> directly from nl80211 to global->nl80211 rather than through this type
> of macro..

It was purely to make the patch smaller.  I like your method
a lot better.

>
> Please take a look at how I split these changes in this series:
> http://w1.fi/p/nl80211-share-netlink/
>
> I think that
> [PATCH 1/6] nl80211: Use a wrapper for genlmsg_put
> and
> [PATCH 4/6] nl80211: Use global netlink rtm event object
> are pretty okay as-is (though, I haven't really tested these at all).
>
> [PATCH 2/6] nl80211: Initialize global context with hostapd
> is kind of ugly and if I understood correctly, missing the global deinit
> call (unless 5/6 handles that somehow).. This should really be moved to
> core hostapd code to get similar design to wpa_supplicant.

I called the de-init on some error paths, but perhaps not on
process shutdown.  I may have lost that code while trying to
split and/or merge the patches.

We could call it when all drivers have been
deleted (sort of like a ref-cnt).

Also, when these initial patches are in, some methods can be re-arranged
so that we do not need forward declarations.

>
> I don't understand why
> [PATCH 3/6] nl80211: Validate global_init/deinit use
> would be needed.

When a driver instance is initialized, we need to create the
global or find the already-created global.  That is what this
code was attempting to accomplish.  Poor man's singleton.

The check for global == global_ptr on deletion was pure paranoia
on my part, just in case there somehow came to be multiple global
structs.  That could be changed to an assert(global == global_ptr)
instead.

> I have not yet fully reviewed
> [PATCH 5/6] nl80211: Use global netlink objects
> but it is at least in clearer state to be reviewed in this form. It
> would be even nicer if some of it like the Probe Request reporting could
> be split to another patch, but I did not look at how easy that would be
> to do.

I am weary of splitting patches, so I'd vote to leave it as is.

A lot of the size of that patch comes from moving the netlink init
logic around, so it's not quite as bad as it looks.

>
> I'm not sure whether I like the
> [PATCH 6/6] nl80211: Add identifier to structs
> change.. I moved it to the end of the series to avoid blocking other
> changes because of this.

If we ever mess up a wpa_msg() or wpa_dbg() call and pass in the wrong context
(or crash for another context casting problem), this patch will allow you to use
gdb on the core file to figure out what the context actually is.

It was very useful to me when I originally did the netlink patch and
the wpa_msg conversions.  But, it is only currently useful for debugging.

Thanks,
Ben




-- 
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com