Re: [B.A.T.M.A.N.] [PATCH] batman-adv: postpone sysfs removal when unregistering

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

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

Hey Antonio,


On Thu, Jan 10, 2013 at 04:34:47AM +0200, Antonio Quartulli wrote:
> Hello Simon,
> 
> thanks a lot for taking care of this and sorry for my very late reply..

no problem, thanks for your review!

> 
> 
> Il 31.12.2012 01:40 Simon Wunderlich ha scritto:
> >diff --git a/hard-interface.c b/hard-interface.c
> >index f1d37cd..eb3a12d 100644
> >--- a/hard-interface.c
> >+++ b/hard-interface.c
> >@@ -506,6 +506,17 @@ out:
> > 	return NULL;
> > }
> >
> >+static void batadv_hardif_remove_interface_finish(struct
> >work_struct *work)
> >+{
> >+	struct batadv_hard_iface *hard_iface;
> >+
> >+	hard_iface = container_of(work, struct batadv_hard_iface,
> >+				  cleanup_work);
> >+
> >+	batadv_sysfs_del_hardif(&hard_iface->hardif_obj);
> >+	batadv_hardif_free_ref(hard_iface);
> >+}
> >+
> > static void batadv_hardif_remove_interface(struct batadv_hard_iface
> >*hard_iface)
> > {
> > 	ASSERT_RTNL();
> >@@ -518,8 +529,9 @@ static void batadv_hardif_remove_interface(struct
> >batadv_hard_iface *hard_iface)
> > 		return;
> >
> > 	hard_iface->if_status = BATADV_IF_TO_BE_REMOVED;
> >-	batadv_sysfs_del_hardif(&hard_iface->hardif_obj);
> >-	batadv_hardif_free_ref(hard_iface);
> >+	INIT_WORK(&hard_iface->cleanup_work,
> >+		  batadv_hardif_remove_interface_finish);
> >+	queue_work(batadv_event_workqueue, &hard_iface->cleanup_work);
> > }
> >
> 
> why do we need to postpone the invocation of
> batadv_hardif_remove_interface_finish() ? Is it also creating a
> possible
> deadlock? As far as I understood only rtnl_lock() would create the
> problem, but it
> is not invoked in batadv_hardif_remove_interface_finish(). It seems
> I am missing
> something?

batadv_hardif_remove_interface() is called by batadv_hard_if_event(),
which is protected rtnl_lock(). You can find an ASSERT_RTNL() there too.
As batadv_hardif_remove_interface() then removes sysfs, this could
be problematic.

> 
> Other than this, remember that the INIT_WORK macro does not need to
> be invoked
> each and every time you want to enqueue the work. It should be
> invoked once only
> (A patch fixing this "issue" for all the delayed works we have in
> the code has
> been recently applied). However it is not a real issue, but we want
> to keep the
> code consistent :)
> 

Well, the work item is actually only initialized once, before taking
the interface down. Then the struct is destroyed, so it can't be called
again. But you are right, for consistency I will move the INIT_WORK
macros to positions where the structs are initialized.

> > int batadv_softif_is_valid(const struct net_device *net_dev)
> >diff --git a/types.h b/types.h
> >index d8061ac..a9800ee 100644
> >--- a/types.h
> >+++ b/types.h
> >@@ -63,6 +63,7 @@ struct batadv_hard_iface {
> > 	struct net_device *soft_iface;
> > 	struct rcu_head rcu;
> > 	struct batadv_hard_iface_bat_iv bat_iv;
> >+	struct work_struct cleanup_work;
> > };
> >
> 
> kernel doc :)
> 

The Mareks kernel doc patch wasn't applied at this time, so I didn't
bother documenting my single new variable, but will do that in the
next revision. :)

Thanks,
	Simon

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