Re: [B.A.T.M.A.N.] [RFC 1/6] batman-adv: Add the initial code for network coding.

[Antonio Quartulli <ordex@autistici.org>]

[-- Attachment #1: Type: text/plain, Size: 3672 bytes --]

On Mon, Nov 19, 2012 at 02:21:59PM +0100, Martin Hundebøll wrote:
...

> +/**
> + * batadv_nc_start_timer - initialise the nc periodic worker
> + * @bat_priv: the bat priv with all the soft interface information
> + */
> +static void batadv_nc_start_timer(struct batadv_priv *bat_priv)
> +{
> +	INIT_DELAYED_WORK(&bat_priv->nc.work, batadv_nc_worker);

INIT_DELAYED_WORK is not really needed each and every time. It can be invoked
when setting up the worker for the first time only. It's pure beautification in
this case. There is a little performance implication but this does not hurt this
code.

> +	queue_delayed_work(batadv_event_workqueue, &bat_priv->nc.work,
> +			   msecs_to_jiffies(10));
> +}
> +
> +/**
> + * batadv_network_coding_init - initialise coding hash table and start

why not keeping the same namespace everywhere? The previous function name starts
with batadv_nc_, now you are moving to batadv_network_coding_.

> + *  house keeping
> + * @bat_priv: the bat priv with all the soft interface information
> + */
> +int batadv_network_coding_init(struct batadv_priv *bat_priv)
> +{

I think you could move the INIT_DELAYED_WORK here..

> +	batadv_nc_start_timer(bat_priv);
> +
> +	return 0;
> +}

[...]

> +/**
> + * batadv_nc_worker - periodic task for house keeping related to network coding
> + * @work: kernel work struct
> + */
> +static void batadv_nc_worker(struct work_struct *work)
> +{
> +	struct delayed_work *delayed_work;
> +	struct batadv_priv_nc *priv_nc;
> +	struct batadv_priv *bat_priv;
> +
> +	delayed_work = container_of(work, struct delayed_work, work);
> +	priv_nc = container_of(delayed_work, struct batadv_priv_nc, work);
> +	bat_priv = container_of(priv_nc, struct batadv_priv, nc);
> +
> +	/* Schedule a new check */
> +	batadv_nc_start_timer(bat_priv);
> +}

What's the meaning of this function? It just runs and reset the worker?


> diff --git a/sysfs-class-net-mesh b/sysfs-class-net-mesh
> index bc41da6..c75e594 100644
> --- a/sysfs-class-net-mesh
> +++ b/sysfs-class-net-mesh
> @@ -74,6 +74,14 @@ Description:
>                  Defines the interval in milliseconds in which batman
>                  sends its protocol messages.
>  
> +What:           /sys/class/net/<mesh_iface>/mesh/network_coding
> +Date:           Nov 2012
> +Contact:        Martin Hundeboll <martin@hundeboll.net>
> +Description:
> +                Controls whether Network Coding (using some magic
> +                to send fewer wifi packets but still the same
> +                content) is enabled or not.
> +

Marek spent a lot of time to sort this list by alphabetical order, make him
happy :)

>  What:           /sys/class/net/<mesh_iface>/mesh/routing_algo
>  Date:           Dec 2011
>  Contact:        Marek Lindner <lindner_marek@yahoo.de>

[...]

> +/**
> + * struct batadv_priv - per mesh interface private data
> + * @network_coding: enable/disable network coding
> + * @batadv_priv_nc: network coding private data
> + */
>  struct batadv_priv {
>  	atomic_t mesh_state;
>  	struct net_device_stats stats;
> @@ -308,6 +321,10 @@ struct batadv_priv {
>  #ifdef CONFIG_BATMAN_ADV_DAT
>  	struct batadv_priv_dat dat;
>  #endif
> +#ifdef CONFIG_BATMAN_ADV_NC
> +	atomic_t network_coding; /* boolean */

just another style note: why don't you put this boolean switch together with the
others?

> +	struct batadv_priv_nc nc;
> +#endif /* CONFIG_BATMAN_ADV_NC */
>  };
>  
>  struct batadv_socket_client {
> -- 
> 1.8.0


Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]