Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Samir Bellabes <sam@synack.fr>]

Hello Evgeniy,

Evgeniy Polyakov <zbr@ioremap.net> writes:
>> +
>> +extern unsigned int event_hash_size;
>> +
>
>> +
>> +static struct list_head *event_hash;
>> +static rwlock_t event_hash_lock = __RW_LOCK_UNLOCKED();
>> +
>
> Those are way too generic names, please use snet_ prefix as you did in
> other places.

done. thank you

> I believe snet should be converted to RCU instead of rw locks.

I see, I will investigated this idea.

>> +/* lookup for a event_hash - before using this function, lock event_hash_lock */
>> +static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall syscall,
>> +						   const u8 protocol)
>> +{
>> +	unsigned int h = 0;
>> +	struct list_head *l;
>> +	struct snet_event_entry *s;
>> +	struct snet_event t;
>> +
>> +	if (!event_hash)
>> +		return NULL;
>> +
>> +	/* building the element to look for */
>> +	t.syscall = syscall;
>> +	t.protocol = protocol;
>> +
>> +	/* computing its hash value */
>> +	h = jhash(&t, sizeof(struct snet_event), 0) % event_hash_size;
>
> You can have better distribution if not simply cut off the remainder.
> Also it is a rather expensive operation.

true, I replaced the modulo operation to a bitmask operations.

>> +	l = &event_hash[h];
>> +
>> +	list_for_each_entry(s, l, list) {
>> +		if ((s->se.protocol == protocol) &&
>> +		    (s->se.syscall == syscall)) {
>> +			return s;
>> +		}
>> +	}
>> +	return NULL;
>> +}
>
> Below comments looks a little bit weird, was it generated automatically
> by editor?

no, it's was a unused debug function. I deleted this comment.

>> +/* void snet_event_dumpall() */
>> +/* { */
>> +/*	unsigned int i = 0; */
>> +/*	struct list_head *l; */
>> +/*	struct snet_event_entry *s; */
>> +
>> +/*	snet_dbg("entering\n"); */
>> +/*	read_lock_bh(&event_hash_lock); */
>> +/*	for (i = 0; i < (event_hash_size - 1); i++) { */
>> +/*		l = &hash[i]; */
>> +/*		list_for_each_entry(s, l, list) { */
>> +/*			snet_dbg("[%d, %d, %d]\n", i, */
>> +/*				 s->se.protocol, s->se.syscall); */
>> +/*		} */
>> +/*	} */
>> +/*	read_unlock_bh(&event_hash_lock); */
>> +/*	snet_dbg("exiting\n"); */
>> +/*	return; */
>> +/* } */
>
>> +int snet_event_insert(const enum snet_syscall syscall, const u8 protocol)
>> +{
>> +	struct snet_event_entry *data = NULL;
>> +	unsigned int h = 0;
>> +
>> +	data = kzalloc(sizeof(struct snet_event_entry), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	write_lock_bh(&event_hash_lock);
>> +	/* check if event is already registered */
>> +	if (!event_hash || __snet_event_lookup(syscall, protocol) != NULL) {
>> +		write_unlock_bh(&event_hash_lock);
>> +		kfree(data);
>> +		return -EINVAL;
>> +	}
>
> What about single error exiting point?

done, thanks

>> +
>> +	data->se.syscall = syscall;
>> +	data->se.protocol = protocol;
>> +	INIT_LIST_HEAD(&(data->list));
>> +	h = jhash(&(data->se), sizeof(struct snet_event), 0) % event_hash_size;
>> +	list_add_tail(&data->list, &event_hash[h]);
>> +	write_unlock_bh(&event_hash_lock);
>> +
>> +	return 0;
>> +}
>
>> +/* init function */
>> +int snet_event_init(void)
>> +{
>> +	int err = 0, i = 0;
>> +
>> +	event_hash = kzalloc(sizeof(struct list_head) * event_hash_size,
>> +			     GFP_KERNEL);
>> +	if (!event_hash) {
>> +		printk(KERN_WARNING
>> +		       "snet: can't alloc memory for event_hash\n");
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	for (i = 0; i < event_hash_size; i++)
>> +		INIT_LIST_HEAD(&(event_hash[i]));
>
> No need for double braces.

again fixed.

thanks for your time Evgeniy,
sam