[2/3] batman-adv: convert batman iv algorithm to use dynamic infrastructure

  Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 bat_iv_ogm.c     |   21 +++++++++++++--------
 bat_ogm.h        |   35 -----------------------------------
 hard-interface.c |   16 ++++++++++------
 routing.c        |    7 +++++--
 send.c           |    8 +++++---
 types.h          |   10 +++++++++-
 6 files changed, 42 insertions(+), 55 deletions(-)
 delete mode 100644 bat_ogm.h
  

> +static struct bat_algo bat_algo_iv __read_mostly = {
>  	.name = "BATMAN IV",
> +	.bat_ogm_init = bat_ogm_init,
> +	.bat_ogm_init_primary = bat_ogm_init_primary,
> +	.bat_ogm_update_mac = bat_ogm_update_mac,
> +	.bat_ogm_schedule = bat_ogm_schedule,
> +	.bat_ogm_emit = bat_ogm_emit,
> +	.bat_ogm_receive = bat_ogm_receive,
>  };

This needs an .owner = THIS_MODULE,

so you can do reference counting on the module.

   Andrew
  

On Tuesday, November 29, 2011 03:02:45 Andrew Lunn wrote:
> > +static struct bat_algo bat_algo_iv __read_mostly = {
> > 
> >  	.name = "BATMAN IV",
> > 
> > +	.bat_ogm_init = bat_ogm_init,
> > +	.bat_ogm_init_primary = bat_ogm_init_primary,
> > +	.bat_ogm_update_mac = bat_ogm_update_mac,
> > +	.bat_ogm_schedule = bat_ogm_schedule,
> > +	.bat_ogm_emit = bat_ogm_emit,
> > +	.bat_ogm_receive = bat_ogm_receive,
> > 
> >  };
> 
> This needs an .owner = THIS_MODULE,
> 
> so you can do reference counting on the module.

It is not a separate module ..

Cheers,
Marek
  

On Tue, Nov 29, 2011 at 03:45:29AM +0800, Marek Lindner wrote:
> On Tuesday, November 29, 2011 03:02:45 Andrew Lunn wrote:
> > > +static struct bat_algo bat_algo_iv __read_mostly = {
> > > 
> > >  	.name = "BATMAN IV",
> > > 
> > > +	.bat_ogm_init = bat_ogm_init,
> > > +	.bat_ogm_init_primary = bat_ogm_init_primary,
> > > +	.bat_ogm_update_mac = bat_ogm_update_mac,
> > > +	.bat_ogm_schedule = bat_ogm_schedule,
> > > +	.bat_ogm_emit = bat_ogm_emit,
> > > +	.bat_ogm_receive = bat_ogm_receive,
> > > 
> > >  };
> > 
> > This needs an .owner = THIS_MODULE,
> > 
> > so you can do reference counting on the module.
> 
> It is not a separate module ..

Which leads to the question, why is it not a separate module?

      Andrew
  

On Tuesday, November 29, 2011 14:09:24 Andrew Lunn wrote:
> On Tue, Nov 29, 2011 at 03:45:29AM +0800, Marek Lindner wrote:
> > On Tuesday, November 29, 2011 03:02:45 Andrew Lunn wrote:
> > > > +static struct bat_algo bat_algo_iv __read_mostly = {
> > > > 
> > > >  	.name = "BATMAN IV",
> > > > 
> > > > +	.bat_ogm_init = bat_ogm_init,
> > > > +	.bat_ogm_init_primary = bat_ogm_init_primary,
> > > > +	.bat_ogm_update_mac = bat_ogm_update_mac,
> > > > +	.bat_ogm_schedule = bat_ogm_schedule,
> > > > +	.bat_ogm_emit = bat_ogm_emit,
> > > > +	.bat_ogm_receive = bat_ogm_receive,
> > > > 
> > > >  };
> > > 
> > > This needs an .owner = THIS_MODULE,
> > > 
> > > so you can do reference counting on the module.
> > 
> > It is not a separate module ..
> 
> Which leads to the question, why is it not a separate module?

Why should it be ? 

Cheers,
Marek
  

On Tue, Nov 29, 2011 at 02:35:09PM +0800, Marek Lindner wrote:
> On Tuesday, November 29, 2011 14:09:24 Andrew Lunn wrote:
> > On Tue, Nov 29, 2011 at 03:45:29AM +0800, Marek Lindner wrote:
> > > On Tuesday, November 29, 2011 03:02:45 Andrew Lunn wrote:
> > > > > +static struct bat_algo bat_algo_iv __read_mostly = {
> > > > > 
> > > > >  	.name = "BATMAN IV",
> > > > > 
> > > > > +	.bat_ogm_init = bat_ogm_init,
> > > > > +	.bat_ogm_init_primary = bat_ogm_init_primary,
> > > > > +	.bat_ogm_update_mac = bat_ogm_update_mac,
> > > > > +	.bat_ogm_schedule = bat_ogm_schedule,
> > > > > +	.bat_ogm_emit = bat_ogm_emit,
> > > > > +	.bat_ogm_receive = bat_ogm_receive,
> > > > > 
> > > > >  };
> > > > 
> > > > This needs an .owner = THIS_MODULE,
> > > > 
> > > > so you can do reference counting on the module.
> > > 
> > > It is not a separate module ..
> > 
> > Which leads to the question, why is it not a separate module?
> 
> Why should it be ? 

I can think of a couple of reasons.

1) You might get asked when you post the code upstream, why you have
the ability to register algorithms, but have not taken it further to
allow modules to load algorithms. That seems to be the normal case in
kernel code, registration implies modules. You probably need an
argument ready why you don't want it.

2) It helps shows you have the split between the routing algorithm and
the rest of the code correct. If you need to add lots of
EXPORT_SYMBOL_GPL() for functions which the routing algorithm needs to
call then maybe the abstraction is wrong?

     Andrew
  

Hi Marek,

interesting approach, thanks for the patches! I don't really think that
we need modules at this point, but we can easily extend it later - for
example, net/mac80211/rate.c has a very similar API and works with modules.

A few comments to the API:

Generally, we should have a few lines per function in types.h to
describe what each function is doing - we don't need that for a RFC patch,
but that would be useful for the final version.

On Tue, Nov 29, 2011 at 12:28:36AM +0800, Marek Lindner wrote:
>  struct bat_algo {
>  	struct hlist_node list;
>  	char name[20];

you can also use a simple pointer here: 
  char *name;

(the name will most probably static in the module anyways)

> +	void (*bat_ogm_init)(struct hard_iface *hard_iface);
> +	void (*bat_ogm_init_primary)(struct hard_iface *hard_iface);
> +	void (*bat_ogm_update_mac)(struct hard_iface *hard_iface);
> +	void (*bat_ogm_schedule)(struct hard_iface *hard_iface,
> +				 int tt_num_changes);

can't we put tt_num_changes somewhere bat_priv?

> +	void (*bat_ogm_emit)(struct forw_packet *forw_packet);
> +	void (*bat_ogm_receive)(const struct ethhdr *ethhdr,
> +				unsigned char *packet_buff, int packet_len,
> +				struct hard_iface *if_incoming);

you can just pass the SKB instead of ethhdr, packet_buff, and packet_len
-> they are derived from the skb anyway.

Then, you have can do the API with always a hard_iface as the first
parameter. ;)

I also agree with Andrew on defining (a subset of) mandatory functions
which every algo has to provide. The register function could then check
and decline algos which don't define mandatory functions, and we don't have
to check availability in the real code later ...

best regards,
	Simon
  

Hi,

> Generally, we should have a few lines per function in types.h to
> describe what each function is doing - we don't need that for a RFC patch,
> but that would be useful for the final version.

ok, I will do that.


> > +	void (*bat_ogm_init)(struct hard_iface *hard_iface);
> > +	void (*bat_ogm_init_primary)(struct hard_iface *hard_iface);
> > +	void (*bat_ogm_update_mac)(struct hard_iface *hard_iface);
> > +	void (*bat_ogm_schedule)(struct hard_iface *hard_iface,
> > +				 int tt_num_changes);
> 
> can't we put tt_num_changes somewhere bat_priv?

tt_num_changes holds the number of changed TT entries since the last OGM. 
Obviously that changes all the time. Are you suggesting we store this value in 
bat_priv instead of a local variable ? 


> > +	void (*bat_ogm_emit)(struct forw_packet *forw_packet);
> > +	void (*bat_ogm_receive)(const struct ethhdr *ethhdr,
> > +				unsigned char *packet_buff, int packet_len,
> > +				struct hard_iface *if_incoming);
> 
> you can just pass the SKB instead of ethhdr, packet_buff, and packet_len
> -> they are derived from the skb anyway.
> 
> Then, you have can do the API with always a hard_iface as the first
> parameter. ;)

I will send a separate patch for this API change in a minute as it is not 
really a part of the dynamic routing algo patchset.

Thanks for all the feedback!

Cheers,
Marek
  

On Mon, Dec 05, 2011 at 04:00:48AM +0800, Marek Lindner wrote:
> > > +	void (*bat_ogm_init)(struct hard_iface *hard_iface);
> > > +	void (*bat_ogm_init_primary)(struct hard_iface *hard_iface);
> > > +	void (*bat_ogm_update_mac)(struct hard_iface *hard_iface);
> > > +	void (*bat_ogm_schedule)(struct hard_iface *hard_iface,
> > > +				 int tt_num_changes);
> > 
> > can't we put tt_num_changes somewhere bat_priv?
> 
> tt_num_changes holds the number of changed TT entries since the last OGM. 
> Obviously that changes all the time. Are you suggesting we store this value in 
> bat_priv instead of a local variable ? 
> 
Yup. I know that it is changed all the time, just as tt_buff and tt_len changes
- it is updated when the packet is generated.

There is not really a technical advantage (or disadvantage), it would just be
more clean if information like this is stored in bat_priv IMHO - we fetch other
stuff like the tt buffer or the ogm from structs like bat_priv or hard interfaces.
And maybe we want to change something in this mechanism in the future.

Just my personal opinion ;)
	Simon