| Message ID | 20210213175148.28375-1-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mld: change context from atomic to sleepable | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2949 this patch: 2949 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 69 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3368 this patch: 3368 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sat, Feb 13, 2021 at 9:52 AM Taehee Yoo <ap420073@gmail.com> wrote: > > The goal of mc_delrec_work delayed work is to call mld_clear_delrec(). > The mld_clear_delrec() is called under both data path and control path. > So, the context of mld_clear_delrec() can be atomic. > But this function accesses struct ifmcaddr6 and struct ip6_sf_list. > These structures are going to be protected by RTNL. > So, this function should be called in a sleepable context. Hmm, but with this patch mld_clear_delrec() is called asynchronously without waiting, is this a problem? If not, please explain why in your changelog. By the way, if you do not use a delay, you can just use regular work. Thanks.
On 21. 2. 14. 오전 4:18, Cong Wang wrote: > On Sat, Feb 13, 2021 at 9:52 AM Taehee Yoo <ap420073@gmail.com> wrote: >> >> The goal of mc_delrec_work delayed work is to call mld_clear_delrec(). >> The mld_clear_delrec() is called under both data path and control path. >> So, the context of mld_clear_delrec() can be atomic. >> But this function accesses struct ifmcaddr6 and struct ip6_sf_list. >> These structures are going to be protected by RTNL. >> So, this function should be called in a sleepable context. > > Hmm, but with this patch mld_clear_delrec() is called asynchronously > without waiting, is this a problem? If not, please explain why in your > changelog. > The mld_clear_delrec() is called when an mld v1 query is received or an interface is down/destroyed. The purpose of this function is to clear deleted records, which are not used when an mld v1 query is received. So, In the datapath, it has no problem. Also it increases the refcount of idev so it has no problem when an interface is down or destroyed. I will include this description in the v3 patch. Thanks! > By the way, if you do not use a delay, you can just use regular work. > > Thanks. >
> > By the way, if you do not use a delay, you can just use regular work. > The regular workqueue API couldn't be used in an atomic context, So I used delayed_work. If 0 delay is passed to delayed_work, it internally calls regular workqueue. So, I think there is no actual difference. Thanks!
diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h index bec372283ac0..5946b5d76f7b 100644 --- a/include/net/if_inet6.h +++ b/include/net/if_inet6.h @@ -182,6 +182,7 @@ struct inet6_dev { struct delayed_work mc_gq_work; /* general query work */ struct delayed_work mc_ifc_work; /* interface change work */ struct delayed_work mc_dad_work; /* dad complete mc work */ + struct delayed_work mc_delrec_work; /* delete records work */ struct ifacaddr6 *ac_list; rwlock_t lock; diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c index 1f7fd3fbb4b6..ca8ca6faca4e 100644 --- a/net/ipv6/mcast.c +++ b/net/ipv6/mcast.c @@ -1067,6 +1067,22 @@ static void mld_dad_stop_work(struct inet6_dev *idev) __in6_dev_put(idev); } +static void mld_clear_delrec_start_work(struct inet6_dev *idev) +{ + write_lock_bh(&idev->lock); + if (!mod_delayed_work(mld_wq, &idev->mc_delrec_work, 0)) + in6_dev_hold(idev); + write_unlock_bh(&idev->lock); +} + +static void mld_clear_delrec_stop_work(struct inet6_dev *idev) +{ + write_lock_bh(&idev->lock); + if (cancel_delayed_work(&idev->mc_delrec_work)) + __in6_dev_put(idev); + write_unlock_bh(&idev->lock); +} + /* * IGMP handling (alias multicast ICMPv6 messages) */ @@ -1304,7 +1320,7 @@ static int mld_process_v1(struct inet6_dev *idev, struct mld_msg *mld, /* cancel the interface change work */ mld_ifc_stop_work(idev); /* clear deleted report items */ - mld_clear_delrec(idev); + mld_clear_delrec_start_work(idev); return 0; } @@ -2120,6 +2136,18 @@ static void mld_dad_work(struct work_struct *work) in6_dev_put(idev); } +static void mld_clear_delrec_work(struct work_struct *work) +{ + struct inet6_dev *idev = container_of(to_delayed_work(work), + struct inet6_dev, + mc_delrec_work); + + rtnl_lock(); + mld_clear_delrec(idev); + rtnl_unlock(); + in6_dev_put(idev); +} + static int ip6_mc_del1_src(struct ifmcaddr6 *pmc, int sfmode, const struct in6_addr *psfsrc) { @@ -2553,6 +2581,7 @@ void ipv6_mc_down(struct inet6_dev *idev) mld_gq_stop_work(idev); mld_dad_stop_work(idev); read_unlock_bh(&idev->lock); + mld_clear_delrec_stop_work(idev); } static void ipv6_mc_reset(struct inet6_dev *idev) @@ -2593,6 +2622,7 @@ void ipv6_mc_init_dev(struct inet6_dev *idev) idev->mc_ifc_count = 0; INIT_DELAYED_WORK(&idev->mc_ifc_work, mld_ifc_work); INIT_DELAYED_WORK(&idev->mc_dad_work, mld_dad_work); + INIT_DELAYED_WORK(&idev->mc_delrec_work, mld_clear_delrec_work); ipv6_mc_reset(idev); write_unlock_bh(&idev->lock); }
The goal of mc_delrec_work delayed work is to call mld_clear_delrec(). The mld_clear_delrec() is called under both data path and control path. So, the context of mld_clear_delrec() can be atomic. But this function accesses struct ifmcaddr6 and struct ip6_sf_list. These structures are going to be protected by RTNL. So, this function should be called in a sleepable context. Suggested-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- v1 -> v2: - Separated from previous big one patch. include/net/if_inet6.h | 1 + net/ipv6/mcast.c | 32 +++++++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-)