Re: [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: debugfs: Add netns support

[Antonio Quartulli <a@unstable.cc>]

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

On Thu, Jan 28, 2016 at 02:40:07AM +0100, Andrew Lunn wrote:
> On Thu, Jan 28, 2016 at 09:28:07AM +0800, Antonio Quartulli wrote:
> > On Wed, Jan 20, 2016 at 06:48:30PM +0100, Andrew Lunn wrote:
> > 
> > [...]
> > 
> > > +static DEFINE_MUTEX(batadv_debugfs_ns_mutex);
> > 
> > [...]
> > 
> > > +static void batadv_debugfs_ns_put(struct net *net)
> > > +{
> > > +	struct batadv_debugfs_ns_entry *ns_entry;
> > > +
> > > +	mutex_lock(&batadv_debugfs_ns_mutex);
> > > +	list_for_each_entry(ns_entry, &batadv_debugfs_ns, link) {
> > > +		if (ns_entry->net == net) {
> > > +			kref_put(&ns_entry->refcount, batadv_ns_entry_release);
> > > +			break;
> > > +		}
> > > +	}
> > > +	mutex_unlock(&batadv_debugfs_ns_mutex);
> > > +}
> > >  
> > 
> > Andrew, is there any particular reason why in this case we use a mutex instead
> > of a spinlock + RCU ? I imagine it is something related to debugfs..or just to
> > make the code simpler as we don't really require a full-blown RCU strategy ?
>  
> I just wanted it to be KISS. We are on a very slow path, only called
> when interfaces are added and removed. Keeping it KISS makes it easier
> to review, make sure i have an unlock for every lock, etc. It is also
> what is the kref documentation suggests.

ok, thanks for explaining, Andrew!
I agree with that, but I wanted to be sure I was understanding the ratio
behind :)


> > how about:
> > 
> > 	debugfs_ns_dir = batadv_debugfs;
> > 
> > > +	if (net != &init_net) {
> > > +		debugfs_ns_dir = batadv_debugfs_ns_get(net);
> > > +		if (!debugfs_ns_dir)
> > > +			goto out;
> > 	}
> > 
> > 	hard_iface->debug_dir = debugfs_create_dir(name, debugfs_ns_dir);
> > 
> > isn't it nicer? :)
> 
> O.K, i will rearrange.

The last chunk reported in my reply could be rearranged the same way.




Thanks a lot!
Other than these points the patch seems sound to me.

Cheers,


-- 
Antonio Quartulli

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