Re: [bisected] Re: Module removal-related regression?

[Jakub Kicinski]

On Mon, 11 Sep 2017 11:29:26 -0700, Dmitry Torokhov wrote:
> On Mon, Sep 11, 2017 at 08:23:32AM -0700, Greg Kroah-Hartman wrote:
> > On Sun, Sep 10, 2017 at 02:22:22PM -0700, Dmitry Torokhov wrote:  
> > > On Sun, Sep 10, 2017 at 12:13 PM, Jakub Kicinski <kubakici@xxxxx> wrote:  
> > > > On Sun, 10 Sep 2017 21:09:08 +0200, Greg Kroah-Hartman wrote:  
> > > >> On Sun, Sep 10, 2017 at 11:12:17AM -0700, Dmitry Torokhov wrote:  
> > > >> > On September 10, 2017 11:00:10 AM PDT, Jakub Kicinski <kubakici@xxxxx> wrote:  
> > > >> > >On Sun, 10 Sep 2017 09:21:11 -0700, Dmitry Torokhov wrote:  
> > > >> > >> On Sun, Sep 10, 2017 at 12:03:38AM +0200, Jakub Kicinski wrote:  
> > > >> > >> > On Sat, 09 Sep 2017 13:59:25 -0700, Dmitry Torokhov wrote:  
> > > >> > >> > > On September 9, 2017 1:17:26 PM PDT, Jakub Kicinski  
> > > >> > ><kubakici@xxxxx> wrote:  
> > > >> > >> > > >On Sat, 9 Sep 2017 12:55:51 -0700, Dmitry Torokhov wrote:  
> > > >> > >> > > >> On Sat, Sep 9, 2017 at 12:27 PM, Jakub Kicinski  
> > > >> > ><kubakici@xxxxx>  
> > > >> > >> > > >wrote:  
> > > >> > >> > > >> > On Sat, 9 Sep 2017 19:41:21 +0200, Jakub Kicinski wrote:  
> > > >> > >  
> > > >> > >> > > >> >> Hi!
> > > >> > >> > > >> >>
> > > >> > >> > > >> >> I'm having trouble with modules on linux/master.  rmmod  
> > > >> > >succeeds  
> > > >> > >> > > >but the  
> > > >> > >> > > >> >> module is still loaded and the refcount goes to 1:
> > > >> > >> > > >> >>
> > > >> > >> > > >> >> #rmmod nfp; insmod ./src/nfp.ko nfp_pf_netdev=0 ; \
> > > >> > >> > > >> >>       /opt/netronome/bin/nfp-hwinfo -n 2  assembly.partno \
> > > >> > >> > > >> >>       lsmod | grep nfp; \
> > > >> > >> > > >> >>       rmmod nfp; \
> > > >> > >> > > >> >>       lsmod | grep nfp
> > > >> > >> > > >> >> nfp                   249856  0
> > > >> > >> > > >> >> nfp                   200704  1
> > > >> > >> > > >> >>
> > > >> > >> > > >> >> If I rmmod again the module will be actually unloaded.  The  
> > > >> > >user  
> > > >> > >> > > >space  
> > > >> > >> > > >> >> is mostly Ubuntu 14.04.  Has anyone seen this?  I'm trying  
> > > >> > >to  
> > > >> > >> > > >bisect  
> > > >> > >> > > >> >> now...  
> > > >> > >> > > >> >
> > > >> > >> > > >> > Got 'em!
> > > >> > >> > > >> >
> > > >> > >> > > >> > commit 1455cf8dbfd06aa7651dcfccbadb7a093944ca65 (HEAD,  
> > > >> > >> > > >refs/bisect/bad)  
> > > >> > >> > > >> > Author: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > > >> > >> > > >> > Date:   Wed Jul 19 17:24:30 2017 -0700
> > > >> > >> > > >> >
> > > >> > >> > > >> >     driver core: emit uevents when device is bound to a  
> > > >> > >driver  
> > > >> > >> > > >>
> > > >> > >> > > >> Does it happen with all modules or only nfp one?
> > > >> > >> > > >>
> > > >> > >> > > >> It seems to work here:
> > > >> > >> > > >>
> > > >> > >> > > >> dtor@dtor-glaptop3:~ $ lsmod | grep psmouse
> > > >> > >> > > >> psmouse               135168  0
> > > >> > >> > > >> dtor@dtor-glaptop3:~ $ sudo rmmod psmouse
> > > >> > >> > > >> dtor@dtor-glaptop3:~ $ lsmod | grep psmouse
> > > >> > >> > > >> dtor@dtor-glaptop3:~ $ sudo modprobe psmouse  
> > > >> > >> > > >
> > > >> > >> > > >It looks like the driver is actually reloaded.  The driver used  
> > > >> > >to  
> > > >> > >> > > >return EPROBE_DEFER, but I think it doesn't any more (rebuilding  
> > > >> > >the  
> > > >> > >> > > >kernel to test that right now).
> > > >> > >> > > >
> > > >> > >> > > >Could the uevent on unbind tickle Ubuntu 14.04's udev or somehow
> > > >> > >> > > >else cause the driver to be loaded again?  
> > > >> > >> > >
> > > >> > >> > > It depends on how silly the udev rules are, but yes, this can  
> > > >> > >definitely happen.  
> > > >> > >> >
> > > >> > >> > I confirmed the driver doesn't use EPROBE_DEFER any more:
> > > >> > >> >
> > > >> > >> > $ grep -nrI EPROBE_DEFER drivers/net/ethernet/netronome/
> > > >> > >> > $  
> > > >> > >>
> > > >> > >> Not sure why you bring the deferrals here, they have nothing to do  
> > > >> > >with  
> > > >> > >> module removal. Also, deferrals are rarely issued by the leaf driver,  
> > > >> > >and  
> > > >> > >> more often by providers of resources (GPIO, regulator, interrupt,  
> > > >> > >etc).
> > > >> > >
> > > >> > >Yes, it's unusual, but this driver used to do it.  Which is exactly why
> > > >> > >I brought it up.  Turns out it was irrelevant :)
> > > >> > >  
> > > >> > >> > I tested without any udev rules in /etc/udev/, just the standard  
> > > >> > >distro  
> > > >> > >> > ones.  Same thing.  
> > > >> > >>
> > > >> > >> Right, so this is the default udev rule:
> > > >> > >>
> > > >> > >> /lib/udev/rules.d/80-drivers.rules:
> > > >> > >>
> > > >> > >> # do not edit this file, it will be overwritten on update
> > > >> > >>
> > > >> > >> ACTION=="remove", GOTO="drivers_end"
> > > >> > >>
> > > >> > >> ENV{MODALIAS}=="?*", RUN{builtin}="kmod load $env{MODALIAS}"  
> > > >>
> > > >> So if the new uevents do not have the MODALIAS line in them, then they
> > > >> will not trigger this?  Dmitry, can you see if that would fix this
> > > >> problem without having to fix everyone's old versions of udev/systemd?  
> > > 
> > > Unfortunately MODALIAS= is being added by individual subsystems having
> > > their subsystem specific format. Unless you'd be OK with
> > > kobject_uevent_env() poking into the generated environment and zapping
> > > MODALIAS= environment variables for KOBJ_BIND/KOBJ_UNBIND actions.  
> > 
> > Hm, any reason why it should be sending these values for those uevents?
> > I guess it's not worth hacking around in the lower levels just for this,
> > to work around crazy userspace stuff.
> >   
> > > I'm still going to submit correction for the rule to systemd folks.  
> > 
> > Yes please.
> >   
> > > > Perhaps another option is dropping the unbind event?  From the commit
> > > > message it seems like only bind is really needed ATM.  Do events have
> > > > to be symmetrical?  
> > > 
> > > While you are absolutely right that bind is the most important one,
> > > I'd be hesitant removing unbind even though we do not have concrete
> > > use case for it yet. The bind operation complements unbind, so having
> > > bind uevent but not unbind "feels weird".  
> > 
> > We might want to disable it for a year or so for people to catch up with
> > a newer version of udev/systemd, and then turn it back on?  
> 
> That is an option, but maybe we could have the patch below for a year or
> 2 instead?
> 
> Jakub, can you try and see if that works for you?

Unfortunately this doesn't seem to solve it :(

# modprobe nfp; lsmod | grep nfp; modprobe -r nfp; lsmod | grep nfp
nfp                  1101824  0 
nfp                  1101824  1