Hi Mihail,
sorry for the very looong delay :) I'm finally sitting at my new location and I can give you some feedback on your nice work. Comments are inline.
On Mon, Jul 08, 2013 at 03:12:40AM +0300, mihail.costea2005@gmail.com wrote:
From: Mihail Costea mihail.costea90@gmail.com
Mades DAT support more types by making its data a void*, adding type field to dat_entry and adding data_type to necessary functions. This change is needed in order to make DAT support any type of data, like IPv6 too.
Adds generic function for transforming DAT data to string. The function is used in order to avoid defining different debug messages for different DAT data types. For example, if we had IPv6 as a DAT data, then "%pI4" should be "%pI6c", but all the other text of the debug message would be the same. Also everything is memorized in a struct in order to avoid further switch cases for all types.
Signed-off-by: Mihail Costea mihail.costea90@gmail.com Signed-off-by: Stefan Popa Stefan.A.Popa@intel.com Reviewed-by: Stefan Popa Stefan.A.Popa@intel.com
distributed-arp-table.c | 197 +++++++++++++++++++++++++++++++++++------------ distributed-arp-table.h | 1 + types.h | 24 +++++- 3 files changed, 169 insertions(+), 53 deletions(-)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index f2543c2..90565d0 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -31,9 +31,32 @@ #include "types.h" #include "translation-table.h"
+static struct batadv_dat_type_info batadv_dat_types_info[] = {
- {
.size = sizeof(__be32),.str_fmt = "%pI4",- },
+};
static void batadv_dat_purge(struct work_struct *work);
/**
- batadv_dat_data_to_str: transforms DAT data to string
- @data: the DAT data
- @type: type of data
- @buf: the buf where the data string is stored
- @buf_len: buf length
- Returns buf.
- */
+static char *batadv_dat_data_to_str(void *data, uint8_t type,
char *buf, size_t buf_len)+{
- snprintf(buf, buf_len, batadv_dat_types_info[type].str_fmt, data);
+return buf;
alignment. Remember to check your patches with "checkpatch.pl --strict" before sending. It will tell you about these hidden mistakes.
+}
+/**
- batadv_dat_start_timer - initialise the DAT periodic worker
- @bat_priv: the bat priv with all the soft interface information
*/ @@ -45,6 +68,19 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv) }
/**
- batadv_dat_entry_free_ref_rcu - free a dat entry using its rcu
- @rcu: the dat entry rcu
- */
+static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu) +{
- struct batadv_dat_entry *dat_entry;
- dat_entry = container_of(rcu, struct batadv_dat_entry, rcu);
- kfree(dat_entry->data);
- kfree(dat_entry);
+}
+/**
- batadv_dat_entry_free_ref - decrement the dat_entry refcounter and possibly
- free it
- @dat_entry: the entry to free
@@ -52,7 +88,7 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv) static void batadv_dat_entry_free_ref(struct batadv_dat_entry *dat_entry) { if (atomic_dec_and_test(&dat_entry->refcount))
kfree_rcu(dat_entry, rcu);
call_rcu(&dat_entry->rcu, batadv_dat_entry_free_ref_rcu);}
since you are not using the kfree_rcu() function anymore, you should also delete the compatibility function we have in compat.c (and its declaration in compat.h).
/** @@ -136,12 +172,21 @@ static void batadv_dat_purge(struct work_struct *work)
- Returns 1 if the two entries are the same, 0 otherwise.
*/ -static int batadv_compare_dat(const struct hlist_node *node, const void *data2) +static int batadv_compare_dat(const struct hlist_node *node, const void *data2) {
- const void *data1 = container_of(node, struct batadv_dat_entry,
hash_entry);
- struct batadv_dat_entry *dat_entry1 =
container_of(node, struct batadv_dat_entry,hash_entry);- struct batadv_dat_entry *dat_entry2 =
container_of(data2,struct batadv_dat_entry, data);
These assignments are really ugly :-P. Please make the assignments after the declarations. They will look much better.
- size_t data_size = batadv_dat_types_info[dat_entry1->type].size;
- return (memcmp(data1, data2, sizeof(__be32)) == 0 ? 1 : 0);
- if (dat_entry1->type != dat_entry2->type)
return 0;- return (memcmp(dat_entry1->data, dat_entry2->data,
data_size) == 0 ? 1 : 0);}
/** @@ -198,8 +243,9 @@ static __be32 batadv_arp_ip_dst(struct sk_buff *skb, int hdr_size) }
/**
- batadv_hash_dat - compute the hash value for an IP address
- batadv_hash_dat - compute the hash value for a DAT data
- @data: data to hash
- @data_type: type of data
- @size: size of the hash table
- Returns the selected index in the hash table for the given data.
@@ -209,7 +255,8 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size) uint32_t hash = 0; const struct batadv_dat_entry *dat = data;
- hash = batadv_hash_bytes(hash, &dat->ip, sizeof(dat->ip));
hash = batadv_hash_bytes(hash, dat->data,
batadv_dat_types_info[dat->type].size);hash = batadv_hash_bytes(hash, &dat->vid, sizeof(dat->vid));
hash += (hash << 3);
@@ -223,32 +270,40 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size)
- batadv_dat_entry_hash_find - look for a given dat_entry in the local hash
- table
- @bat_priv: the bat priv with all the soft interface information
- @ip: search key
- @data: search key
*/
- @data_type: type of data
- @vid: VLAN identifier
- Returns the dat_entry if found, NULL otherwise.
static struct batadv_dat_entry * -batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip,
unsigned short vid)+batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *data,
uint8_t data_type, unsigned short vid){ struct hlist_head *head; struct batadv_dat_entry to_find, *dat_entry, *dat_entry_tmp = NULL; struct batadv_hashtable *hash = bat_priv->dat.hash;
- uint32_t index;
uint32_t index, data_size = batadv_dat_types_info[data_type].size;
if (!hash) return NULL;
- to_find.ip = ip;
- to_find.data = kmalloc(data_size, GFP_ATOMIC);
- if (!to_find.data)
return NULL;- memcpy(to_find.data, data, data_size);
why do you create a copy of the data? It is going to be read only by the hashing function. I think you can reuse the same data passed as argument no? You can directly assign "to_find.data = data;", can't you?
to_find.type = data_type; to_find.vid = vid;
index = batadv_hash_dat(&to_find, hash->size); head = &hash->table[index];
kfree(to_find.data);
..consequently, this kfree can be deleted.
rcu_read_lock(); hlist_for_each_entry_rcu(dat_entry, head, hash_entry) {
if (dat_entry->ip != ip)
if (dat_entry->type != data_type)continue;if (memcmp(dat_entry->data, data, data_size)) continue;if (!atomic_inc_not_zero(&dat_entry->refcount))
@@ -265,25 +320,30 @@ batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip, /**
- batadv_dat_entry_add - add a new dat entry or update it if already exists
- @bat_priv: the bat priv with all the soft interface information
- @ip: ipv4 to add/edit
- @mac_addr: mac address to assign to the given ipv4
- @data: the data to add/edit
- @data_type: type of the data added to DAT
*/
- @mac_addr: mac address to assign to the given data
- @vid: VLAN identifier
-static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip,
uint8_t *mac_addr, unsigned short vid)+static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data,
uint8_t data_type, uint8_t *mac_addr,unsigned short vid)
here I realised we have a small conceptual problem. what we are calling "data" is not the real data, but is the "key". The data instead is supposed to be what we store in the entry: the mac address (that could possibly be generalised too..).
I think generalising the data is not very important now. We have an ARP/ND table for now, so we can assume we only want to store MAC addresses, but I would suggest to rename the member (and so the variables) from "data" to "key". It makes much more sense in terms of DHT. (Tell me if you think I am not right).
From now on I'll try to speed up the reviews so that we can get this thing done soonish ;)
Cheers,