lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAOMdWSKU_Ezk-15whDnNQKK_is2UtBOY59_4fPfKZE0-K+cB6w@mail.gmail.com>
Date: Mon, 15 Jul 2024 10:50:06 -0700
From: Allen <allen.lkml@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: kuba@...nel.org, Guo-Fu Tseng <cooldavid@...ldavid.org>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, jes@...ined-monkey.org, 
	kda@...ux-powerpc.org, cai.huoqing@...ux.dev, dougmill@...ux.ibm.com, 
	npiggin@...il.com, christophe.leroy@...roup.eu, aneesh.kumar@...nel.org, 
	naveen.n.rao@...ux.ibm.com, nnac123@...ux.ibm.com, tlfalcon@...ux.ibm.com, 
	marcin.s.wojtas@...il.com, mlindner@...vell.com, stephen@...workplumber.org, 
	nbd@....name, sean.wang@...iatek.com, Mark-MC.Lee@...iatek.com, 
	lorenzo@...nel.org, matthias.bgg@...il.com, 
	angelogioacchino.delregno@...labora.com, borisp@...dia.com, 
	bryan.whitehead@...rochip.com, UNGLinuxDriver@...rochip.com, 
	louis.peens@...igine.com, richardcochran@...il.com, 
	linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-acenic@...site.dk, linux-net-drivers@....com, netdev@...r.kernel.org
Subject: Re: [PATCH 13/15] net: jme: Convert tasklet API to new bottom half
 workqueue mechanism

> > > > @@ -1326,22 +1326,22 @@ static void jme_link_change_work(struct work_struct *work)
> > > >               jme_start_shutdown_timer(jme);
> > > >       }
> > > >
> > > > -     goto out_enable_tasklet;
> > > > +     goto out_enable_bh_work;
> > > >
> > > >  err_out_free_rx_resources:
> > > >       jme_free_rx_resources(jme);
> > > > -out_enable_tasklet:
> > > > -     tasklet_enable(&jme->txclean_task);
> > > > -     tasklet_enable(&jme->rxclean_task);
> > > > -     tasklet_enable(&jme->rxempty_task);
> > > > +out_enable_bh_work:
> > > > +     enable_and_queue_work(system_bh_wq, &jme->txclean_bh_work);
> > > > +     enable_and_queue_work(system_bh_wq, &jme->rxclean_bh_work);
> > > > +     enable_and_queue_work(system_bh_wq, &jme->rxempty_bh_work);
> > >
> > > This will unconditionally schedule the rxempty_bh_work and is AFAICS a
> > > different behavior WRT prior this patch.
> > >
> > > In turn the rxempty_bh_work() will emit (almost unconditionally) the
> > > 'RX Queue Full!' message, so the change should be visibile to the user.
> > >
> > > I think you should queue the work only if it was queued at cancel time.
> > > You likely need additional status to do that.
> > >
> >
> >  Thank you for taking the time out to review. Now that it's been a week, I was
> > preparing to send out version 3. Before I do that, I want to make sure if this
> > the below approach is acceptable.
>
> I _think_ the following does not track the  rxempty_bh_work 'queued'
> status fully/correctly.
>
> > @@ -1282,9 +1282,9 @@ static void jme_link_change_work(struct work_struct *work)
> >                 jme_stop_shutdown_timer(jme);
> >
> >         jme_stop_pcc_timer(jme);
> > -       tasklet_disable(&jme->txclean_task);
> > -       tasklet_disable(&jme->rxclean_task);
> > -       tasklet_disable(&jme->rxempty_task);
> > +       disable_work_sync(&jme->txclean_bh_work);
> > +       disable_work_sync(&jme->rxclean_bh_work);
> > +       disable_work_sync(&jme->rxempty_bh_work);
>
> I think the above should be:
>
>           jme->rxempty_bh_work_queued = disable_work_sync(&jme->rxempty_bh_work);
>
> [...]
> > @@ -1326,22 +1326,23 @@ static void jme_link_change_work(struct
> > work_struct *work)
> >                 jme_start_shutdown_timer(jme);
> >         }
> >
> > -       goto out_enable_tasklet;
> > +       goto out_enable_bh_work;
> >
> >  err_out_free_rx_resources:
> >         jme_free_rx_resources(jme);
> > -out_enable_tasklet:
> > -       tasklet_enable(&jme->txclean_task);
> > -       tasklet_enable(&jme->rxclean_task);
> > -       tasklet_enable(&jme->rxempty_task);
> > +out_enable_bh_work:
> > +       enable_and_queue_work(system_bh_wq, &jme->txclean_bh_work);
> > +       enable_and_queue_work(system_bh_wq, &jme->rxclean_bh_work);
> > +       if (jme->rxempty_bh_work_queued)
> > +               enable_and_queue_work(system_bh_wq, &jme->rxempty_bh_work);
>
> Missing:
>
>           else
>                 enable_work(system_bh_wq, &jme->rxempty_bh_work);
>
> [...]
> > @@ -3180,9 +3182,9 @@ jme_suspend(struct device *dev)
> >         netif_stop_queue(netdev);
> >         jme_stop_irq(jme);
> >
> > -       tasklet_disable(&jme->txclean_task);
> > -       tasklet_disable(&jme->rxclean_task);
> > -       tasklet_disable(&jme->rxempty_task);
> > +       disable_work_sync(&jme->txclean_bh_work);
> > +       disable_work_sync(&jme->rxclean_bh_work);
> > +       disable_work_sync(&jme->rxempty_bh_work);
>
> should be:
>
>           jme->rxempty_bh_work_queued = disable_work_sync(&jme->rxempty_bh_work);
>
>
> >
> > @@ -3198,9 +3200,10 @@ jme_suspend(struct device *dev)
> >                 jme->phylink = 0;
> >         }
> >
> > -       tasklet_enable(&jme->txclean_task);
> > -       tasklet_enable(&jme->rxclean_task);
> > -       tasklet_enable(&jme->rxempty_task);
> > +       enable_and_queue_work(system_bh_wq, &jme->txclean_bh_work);
> > +       enable_and_queue_work(system_bh_wq, &jme->rxclean_bh_work);
> > +       jme->rxempty_bh_work_queued = true;
> > +       enable_and_queue_work(system_bh_wq, &jme->rxempty_bh_work);
>
> should be:
>
>         if (jme->rxempty_bh_work_queued)
>                 enable_and_queue_work(system_bh_wq, &jme->rxempty_bh_work);
>         else
>                 enable_work(system_bh_wq, &jme->rxempty_bh_work);
>
> I think the above ones are the only places where you need to touch
> 'rxempty_bh_work_queued'.
>
>
> [...]
> >   Do we need a flag for rxclean and txclean too?
>
> Functionally speaking I don't think it will be necessary, as
> rxclean_bh_work() and txclean_bh_work() don't emit warnings on spurious
> invocation.
>
> Thanks,
>
> Paolo
>

Thank you very much. Will send out v3 later today with these changes.
Note, it will be as follows, enable_work() does not have workqueue type.

+  if (jme->rxempty_bh_work_queued)
+                 enable_and_queue_work(system_bh_wq, &jme->rxempty_bh_work);
+         else
-                 enable_work(system_bh_wq, &jme->rxempty_bh_work);
+                enable_work(&jme->rxempty_bh_work);

Thanks,
Allen
>


-- 
       - Allen

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ