On Fri, Sep 20, 2024 at 02:07:19PM +0200, Remi Pommarel wrote: > On Fri, Sep 20, 2024 at 12:00:08PM +0200, Johannes Berg wrote: > > On Fri, 2024-09-20 at 10:41 +0200, Remi Pommarel wrote: > > > > I think using timer_pending(&link->color_collision_detect_work->timer) > > > > to replace delayed_work_pending(), even if the semantic is a bit > > > > different, would be ok to fulfill the rate limiting purpose. > > > > I think you're right. We could as well check list_empty() or so, but it > > wouldn't make a big difference. As you say: > > > > > > Having the > > > > same delayed_work_pending() semantics on wiphy work queue would require > > > > to take wiphy lock > > > > To really have it known _precisely_, that's true. > > > > > > which seem a bit superfluous here. > > > > It's actually simply also not possible - if we could sleep in > > ieee80211_obss_color_collision_notify() and take the wiphy mutex, we'd > > probably have done this completely differently :) > > > > And a hypothetical wiphy_delayed_work_pending() API should therefore not > > be required to be called with the wiphy mutex held. > > > > I think that perhaps we should add such a trivial inline instead of > > open-coding the timer_pending() check, but I'm not sure whether or not > > it should also check the list (i.e. check if timer has expired, but work > > hasn't started yet): on the one hand it seems more appropriate, and if > > actually holding the wiphy mutex it would in fact be completely correct, > > on the other hand maybe it encourages being sloppily thinking the return > > value is always perfect? > > > > Right now I'd tend to have the check and document that it's only > > guaranteed when under the wiphy mutex. > > I had this exact train of thought and was replying that I did agree on > that, and then I checked the other *_pending semantics for which I tend > to forget the details. IIUC timer_pending() and work_pending() can both > return false if callback is still running (or even not yet started). > Thus the hypothetical wiphy_work_pending() semantics could allow to > implement it using timer_pending(). > > Adding a list_empty() check while making it more "precise" does not make > it "perfect" (even if there is no clear notion of what perfection should > be here) even with wiphy lock held. Indeed if wiphy_work_pending() is > called precisely after delayed work timer has cleared its pending state > but before the callback (i.e wiphy_delayed_work_timer()) has added the > work in the list both !timer_pending() and list_empty(work->entry) would > be true. So there is a small window where wiphy_work_pending() wouldn't > be more precise than just checking timer_pending() as show below: > > CPU0 CPU1 > expire_timers > detach_timer > wiphy_work_pending() -> return false > timer->function > wiphy_work_queue > list_add_tail() > wiphy_work_pending() -> return false I meant wiphy_work_pending() -> return true here ^. -- Remi
