| Message ID | 20190214074712.17846-2-vladbu@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Refactor flower classifier to remove dependency on rtnl lock | expand |
On Wed, Feb 13, 2019 at 11:47 PM Vlad Buslov <vladbu@mellanox.com> wrote: > > Flower classifier only changes root pointer during init and destroy. Cls > API implements reference counting for tcf_proto, so there is no danger of > concurrent access to tp when it is being destroyed, even without protection > provided by rtnl lock. How about atomicity? Refcnt doesn't guarantee atomicity, how do you make sure two concurrent modifications are atomic? > > Implement new function fl_head_dereference() to dereference tp->root > without checking for rtnl lock. Use it in all flower function that obtain > head pointer instead of rtnl_dereference(). > So what lock protects RCU writers after this patch?
On Mon 18 Feb 2019 at 19:08, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Wed, Feb 13, 2019 at 11:47 PM Vlad Buslov <vladbu@mellanox.com> wrote: >> >> Flower classifier only changes root pointer during init and destroy. Cls >> API implements reference counting for tcf_proto, so there is no danger of >> concurrent access to tp when it is being destroyed, even without protection >> provided by rtnl lock. > > How about atomicity? Refcnt doesn't guarantee atomicity, how do > you make sure two concurrent modifications are atomic? In order to guarantee atomicity I lock shared flower classifier data structures with tp->lock in following patches. > > >> >> Implement new function fl_head_dereference() to dereference tp->root >> without checking for rtnl lock. Use it in all flower function that obtain >> head pointer instead of rtnl_dereference(). >> > > So what lock protects RCU writers after this patch? I explained it in comment for fl_head_dereference(), but should have copied this information to changelog as well: Flower classifier only changes root pointer during init and destroy. Cls API implements reference counting for tcf_proto, so there is no danger of concurrent access to tp when it is being destroyed, even without protection provided by rtnl lock. In initial version of this change I used tp->lock to protect tp->root access and verified it with lockdep, but during internal review Jiri noted that this is not needed in current flower implementation.
On Tue, Feb 19, 2019 at 1:46 AM Vlad Buslov <vladbu@mellanox.com> wrote: > > > On Mon 18 Feb 2019 at 19:08, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Wed, Feb 13, 2019 at 11:47 PM Vlad Buslov <vladbu@mellanox.com> wrote: > >> > >> Flower classifier only changes root pointer during init and destroy. Cls > >> API implements reference counting for tcf_proto, so there is no danger of > >> concurrent access to tp when it is being destroyed, even without protection > >> provided by rtnl lock. > > > > How about atomicity? Refcnt doesn't guarantee atomicity, how do > > you make sure two concurrent modifications are atomic? > > In order to guarantee atomicity I lock shared flower classifier data > structures with tp->lock in following patches. Sure, I meant the atomicity of the _whole_ change, as you know the TC filters are stored in hierarchical structures: a block, a chain, a tp struct, some type-specific data structure like a hash table. Locking tp only solves a partial of the atomicity here. Are you going to restart the whole change from top down to the bottom? > > > > > > >> > >> Implement new function fl_head_dereference() to dereference tp->root > >> without checking for rtnl lock. Use it in all flower function that obtain > >> head pointer instead of rtnl_dereference(). > >> > > > > So what lock protects RCU writers after this patch? > > I explained it in comment for fl_head_dereference(), but should have > copied this information to changelog as well: > Flower classifier only changes root pointer during init and destroy. > Cls API implements reference counting for tcf_proto, so there is no > danger of concurrent access to tp when it is being destroyed, even > without protection provided by rtnl lock. So you are saying an RCU pointer is okay to deference without any lock eve without RCU read lock, right? > > In initial version of this change I used tp->lock to protect tp->root > access and verified it with lockdep, but during internal review Jiri > noted that this is not needed in current flower implementation. Let's see what you have on top of your own branch unlocked_flower_cong_1: 1458 static int fl_change(struct net *net, struct sk_buff *in_skb, 1459 struct tcf_proto *tp, unsigned long base, 1460 u32 handle, struct nlattr **tca, 1461 void **arg, bool ovr, bool rtnl_held, 1462 struct netlink_ext_ack *extack) 1463 { 1464 struct cls_fl_head *head = fl_head_dereference(tp); At the point of line 1464, there is no lock taken, tp->lock is taken after it, block->lock or chain lock is already unlocked before ->change(). So, what protects this RCU structure? According to RCU, it must be either RCU read lock or some writer lock. I see none here. :( What am I missing?
On Wed 20 Feb 2019 at 22:33, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Tue, Feb 19, 2019 at 1:46 AM Vlad Buslov <vladbu@mellanox.com> wrote: >> >> >> On Mon 18 Feb 2019 at 19:08, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> > On Wed, Feb 13, 2019 at 11:47 PM Vlad Buslov <vladbu@mellanox.com> wrote: >> >> >> >> Flower classifier only changes root pointer during init and destroy. Cls >> >> API implements reference counting for tcf_proto, so there is no danger of >> >> concurrent access to tp when it is being destroyed, even without protection >> >> provided by rtnl lock. >> > >> > How about atomicity? Refcnt doesn't guarantee atomicity, how do >> > you make sure two concurrent modifications are atomic? >> >> In order to guarantee atomicity I lock shared flower classifier data >> structures with tp->lock in following patches. > > Sure, I meant the atomicity of the _whole_ change, as you know > the TC filters are stored in hierarchical structures: a block, a chain, > a tp struct, some type-specific data structure like a hash table. > > Locking tp only solves a partial of the atomicity here. Are you > going to restart the whole change from top down to the bottom? You mean in cases where there is a conflict? Yes, processing EAGAIN in tc_new_tfilter() involves releasing and re-obtaining all locks and references. > > >> >> > >> > >> >> >> >> Implement new function fl_head_dereference() to dereference tp->root >> >> without checking for rtnl lock. Use it in all flower function that obtain >> >> head pointer instead of rtnl_dereference(). >> >> >> > >> > So what lock protects RCU writers after this patch? >> >> I explained it in comment for fl_head_dereference(), but should have >> copied this information to changelog as well: >> Flower classifier only changes root pointer during init and destroy. >> Cls API implements reference counting for tcf_proto, so there is no >> danger of concurrent access to tp when it is being destroyed, even >> without protection provided by rtnl lock. > > So you are saying an RCU pointer is okay to deference without > any lock eve without RCU read lock, right? > > >> >> In initial version of this change I used tp->lock to protect tp->root >> access and verified it with lockdep, but during internal review Jiri >> noted that this is not needed in current flower implementation. > > Let's see what you have on top of your own branch > unlocked_flower_cong_1: > > 1458 static int fl_change(struct net *net, struct sk_buff *in_skb, > 1459 struct tcf_proto *tp, unsigned long base, > 1460 u32 handle, struct nlattr **tca, > 1461 void **arg, bool ovr, bool rtnl_held, > 1462 struct netlink_ext_ack *extack) > 1463 { > 1464 struct cls_fl_head *head = fl_head_dereference(tp); > > At the point of line 1464, there is no lock taken, tp->lock is taken > after it, block->lock or chain lock is already unlocked before ->change(). > > So, what protects this RCU structure? According to RCU, it must be > either RCU read lock or some writer lock. I see none here. :( > > What am I missing? Initially I had flower implementation that protected tp->root access with tp->lock, re-obtaining lock and head reference each time it was used, sometimes multiple times per function (head reference is usually obtained in beginning of every flower API function and used multiple times afterwards). This complicated the code and reduced parallelism. However, in case of flower classifier tp->root is never reassigned after init, so essentially there is no "CU" part in this "RCU" pointer. This realization allowed me to refactor unlocked flower code to look much nicer and perform better. Am I missing something in flower classifier implementation?
On Thu, Feb 21, 2019 at 9:45 AM Vlad Buslov <vladbu@mellanox.com> wrote: > > > On Wed 20 Feb 2019 at 22:33, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Tue, Feb 19, 2019 at 1:46 AM Vlad Buslov <vladbu@mellanox.com> wrote: > >> > >> > >> On Mon 18 Feb 2019 at 19:08, Cong Wang <xiyou.wangcong@gmail.com> wrote: > >> > On Wed, Feb 13, 2019 at 11:47 PM Vlad Buslov <vladbu@mellanox.com> wrote: > >> >> > >> >> Flower classifier only changes root pointer during init and destroy. Cls > >> >> API implements reference counting for tcf_proto, so there is no danger of > >> >> concurrent access to tp when it is being destroyed, even without protection > >> >> provided by rtnl lock. > >> > > >> > How about atomicity? Refcnt doesn't guarantee atomicity, how do > >> > you make sure two concurrent modifications are atomic? > >> > >> In order to guarantee atomicity I lock shared flower classifier data > >> structures with tp->lock in following patches. > > > > Sure, I meant the atomicity of the _whole_ change, as you know > > the TC filters are stored in hierarchical structures: a block, a chain, > > a tp struct, some type-specific data structure like a hash table. > > > > Locking tp only solves a partial of the atomicity here. Are you > > going to restart the whole change from top down to the bottom? > > You mean in cases where there is a conflict? Yes, processing EAGAIN in > tc_new_tfilter() involves releasing and re-obtaining all locks and > references. Sure, restart only happens when a conflict is detected, this is why I called it worst scenario. > > > > > > >> > >> > > >> > > >> >> > >> >> Implement new function fl_head_dereference() to dereference tp->root > >> >> without checking for rtnl lock. Use it in all flower function that obtain > >> >> head pointer instead of rtnl_dereference(). > >> >> > >> > > >> > So what lock protects RCU writers after this patch? > >> > >> I explained it in comment for fl_head_dereference(), but should have > >> copied this information to changelog as well: > >> Flower classifier only changes root pointer during init and destroy. > >> Cls API implements reference counting for tcf_proto, so there is no > >> danger of concurrent access to tp when it is being destroyed, even > >> without protection provided by rtnl lock. > > > > So you are saying an RCU pointer is okay to deference without > > any lock eve without RCU read lock, right? > > > > > >> > >> In initial version of this change I used tp->lock to protect tp->root > >> access and verified it with lockdep, but during internal review Jiri > >> noted that this is not needed in current flower implementation. > > > > Let's see what you have on top of your own branch > > unlocked_flower_cong_1: > > > > 1458 static int fl_change(struct net *net, struct sk_buff *in_skb, > > 1459 struct tcf_proto *tp, unsigned long base, > > 1460 u32 handle, struct nlattr **tca, > > 1461 void **arg, bool ovr, bool rtnl_held, > > 1462 struct netlink_ext_ack *extack) > > 1463 { > > 1464 struct cls_fl_head *head = fl_head_dereference(tp); > > > > At the point of line 1464, there is no lock taken, tp->lock is taken > > after it, block->lock or chain lock is already unlocked before ->change(). > > > > So, what protects this RCU structure? According to RCU, it must be > > either RCU read lock or some writer lock. I see none here. :( > > > > What am I missing? > > Initially I had flower implementation that protected tp->root access > with tp->lock, re-obtaining lock and head reference each time it was > used, sometimes multiple times per function (head reference is usually > obtained in beginning of every flower API function and used multiple > times afterwards). This complicated the code and reduced parallelism. > However, in case of flower classifier tp->root is never reassigned after > init, so essentially there is no "CU" part in this "RCU" pointer. This > realization allowed me to refactor unlocked flower code to look much > nicer and perform better. Am I missing something in flower classifier > implementation? So if it is no longer RCU any more, why do you still use rcu_dereference_protected()? That is, why not just deref it as a raw pointer? And, I don't think I can buy your argument here. The RCU infrastructure should not be changed even after your patches, the fast path is still protocted by RCU read lock, while the slow path now is protected by some smaller-scope locks. What makes cls_flower so unique that it doesn't even need RCU here? tp->root is not reassigned but it is still freed via RCU infra, that is in fl_destroy_sleepable(). Thanks.
On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Thu, Feb 21, 2019 at 9:45 AM Vlad Buslov <vladbu@mellanox.com> wrote: >> >> >> On Wed 20 Feb 2019 at 22:33, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> > On Tue, Feb 19, 2019 at 1:46 AM Vlad Buslov <vladbu@mellanox.com> wrote: >> >> >> >> >> >> On Mon 18 Feb 2019 at 19:08, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> > On Wed, Feb 13, 2019 at 11:47 PM Vlad Buslov <vladbu@mellanox.com> wrote: >> >> >> >> >> >> Flower classifier only changes root pointer during init and destroy. Cls >> >> >> API implements reference counting for tcf_proto, so there is no danger of >> >> >> concurrent access to tp when it is being destroyed, even without protection >> >> >> provided by rtnl lock. >> >> > >> >> > How about atomicity? Refcnt doesn't guarantee atomicity, how do >> >> > you make sure two concurrent modifications are atomic? >> >> >> >> In order to guarantee atomicity I lock shared flower classifier data >> >> structures with tp->lock in following patches. >> > >> > Sure, I meant the atomicity of the _whole_ change, as you know >> > the TC filters are stored in hierarchical structures: a block, a chain, >> > a tp struct, some type-specific data structure like a hash table. >> > >> > Locking tp only solves a partial of the atomicity here. Are you >> > going to restart the whole change from top down to the bottom? >> >> You mean in cases where there is a conflict? Yes, processing EAGAIN in >> tc_new_tfilter() involves releasing and re-obtaining all locks and >> references. > > Sure, restart only happens when a conflict is detected, this is > why I called it worst scenario. > > >> >> > >> > >> >> >> >> > >> >> > >> >> >> >> >> >> Implement new function fl_head_dereference() to dereference tp->root >> >> >> without checking for rtnl lock. Use it in all flower function that obtain >> >> >> head pointer instead of rtnl_dereference(). >> >> >> >> >> > >> >> > So what lock protects RCU writers after this patch? >> >> >> >> I explained it in comment for fl_head_dereference(), but should have >> >> copied this information to changelog as well: >> >> Flower classifier only changes root pointer during init and destroy. >> >> Cls API implements reference counting for tcf_proto, so there is no >> >> danger of concurrent access to tp when it is being destroyed, even >> >> without protection provided by rtnl lock. >> > >> > So you are saying an RCU pointer is okay to deference without >> > any lock eve without RCU read lock, right? >> > >> > >> >> >> >> In initial version of this change I used tp->lock to protect tp->root >> >> access and verified it with lockdep, but during internal review Jiri >> >> noted that this is not needed in current flower implementation. >> > >> > Let's see what you have on top of your own branch >> > unlocked_flower_cong_1: >> > >> > 1458 static int fl_change(struct net *net, struct sk_buff *in_skb, >> > 1459 struct tcf_proto *tp, unsigned long base, >> > 1460 u32 handle, struct nlattr **tca, >> > 1461 void **arg, bool ovr, bool rtnl_held, >> > 1462 struct netlink_ext_ack *extack) >> > 1463 { >> > 1464 struct cls_fl_head *head = fl_head_dereference(tp); >> > >> > At the point of line 1464, there is no lock taken, tp->lock is taken >> > after it, block->lock or chain lock is already unlocked before ->change(). >> > >> > So, what protects this RCU structure? According to RCU, it must be >> > either RCU read lock or some writer lock. I see none here. :( >> > >> > What am I missing? >> >> Initially I had flower implementation that protected tp->root access >> with tp->lock, re-obtaining lock and head reference each time it was >> used, sometimes multiple times per function (head reference is usually >> obtained in beginning of every flower API function and used multiple >> times afterwards). This complicated the code and reduced parallelism. >> However, in case of flower classifier tp->root is never reassigned after >> init, so essentially there is no "CU" part in this "RCU" pointer. This >> realization allowed me to refactor unlocked flower code to look much >> nicer and perform better. Am I missing something in flower classifier >> implementation? > > So if it is no longer RCU any more, why do you still use > rcu_dereference_protected()? That is, why not just deref it as a raw > pointer? > > And, I don't think I can buy your argument here. The RCU infrastructure > should not be changed even after your patches, the fast path is still > protocted by RCU read lock, while the slow path now is protected by > some smaller-scope locks. What makes cls_flower so unique that > it doesn't even need RCU here? tp->root is not reassigned but it is still > freed via RCU infra, that is in fl_destroy_sleepable(). > > Thanks. My cls API patch set introduced reference counting for tcf_proto structure. With that change tp->ops->destroy() (which calls fl_destroy() and fl_destroy_sleepable(), in case of flower classifier) is only called after last reference to tp is released. All slow path users of tp->ops must obtain reference to tp, so concurrent call to fl_destroy() is not possible. Before this change tcf_proto structure didn't have reference counting support and required users to obtain rtnl mutex before calling its ops callbacks. This was verified in flower by using rtnl_dereference to obtain tp->root.
On Mon, Feb 25, 2019 at 8:11 AM Vlad Buslov <vladbu@mellanox.com> wrote: > > > On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > So if it is no longer RCU any more, why do you still use > > rcu_dereference_protected()? That is, why not just deref it as a raw > > pointer? Any answer for this question? > > > > And, I don't think I can buy your argument here. The RCU infrastructure > > should not be changed even after your patches, the fast path is still > > protocted by RCU read lock, while the slow path now is protected by > > some smaller-scope locks. What makes cls_flower so unique that > > it doesn't even need RCU here? tp->root is not reassigned but it is still > > freed via RCU infra, that is in fl_destroy_sleepable(). > > > > Thanks. > > My cls API patch set introduced reference counting for tcf_proto > structure. With that change tp->ops->destroy() (which calls fl_destroy() > and fl_destroy_sleepable(), in case of flower classifier) is only called > after last reference to tp is released. All slow path users of tp->ops > must obtain reference to tp, so concurrent call to fl_destroy() is not > possible. Before this change tcf_proto structure didn't have reference > counting support and required users to obtain rtnl mutex before calling > its ops callbacks. This was verified in flower by using rtnl_dereference > to obtain tp->root. Yes, but fast path doesn't hold a refnct of tp, does it? If not, you still rely on RCU for sync with readers. If yes, then probably RCU can be gone. Now you are in a middle of the two, that is taking RCU read lock on fast path without a refcnt, meanwhile still uses rcu_dereference on slow paths without any lock. For me, you at least don't use the RCU API correctly here. Thanks.
On Mon 25 Feb 2019 at 22:39, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Mon, Feb 25, 2019 at 8:11 AM Vlad Buslov <vladbu@mellanox.com> wrote: >> >> >> On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> > >> > So if it is no longer RCU any more, why do you still use >> > rcu_dereference_protected()? That is, why not just deref it as a raw >> > pointer? > > > Any answer for this question? I decided that since there is neither possibility of concurrent pointer assignment nor deallocation of object that it points to, most performant solution would be using rcu_dereference_protected() which is the only RCU dereference helper that doesn't use READ_ONCE. I now understand that this is confusing (and most likely doesn't provide any noticeable performance improvement anyway!) and will change this patch to use rcu_dereference_raw() as you suggest. > > >> > >> > And, I don't think I can buy your argument here. The RCU infrastructure >> > should not be changed even after your patches, the fast path is still >> > protocted by RCU read lock, while the slow path now is protected by >> > some smaller-scope locks. What makes cls_flower so unique that >> > it doesn't even need RCU here? tp->root is not reassigned but it is still >> > freed via RCU infra, that is in fl_destroy_sleepable(). >> > >> > Thanks. >> >> My cls API patch set introduced reference counting for tcf_proto >> structure. With that change tp->ops->destroy() (which calls fl_destroy() >> and fl_destroy_sleepable(), in case of flower classifier) is only called >> after last reference to tp is released. All slow path users of tp->ops >> must obtain reference to tp, so concurrent call to fl_destroy() is not >> possible. Before this change tcf_proto structure didn't have reference >> counting support and required users to obtain rtnl mutex before calling >> its ops callbacks. This was verified in flower by using rtnl_dereference >> to obtain tp->root. > > Yes, but fast path doesn't hold a refnct of tp, does it? If not, you still > rely on RCU for sync with readers. If yes, then probably RCU can be > gone. > > Now you are in a middle of the two, that is taking RCU read lock on > fast path without a refcnt, meanwhile still uses rcu_dereference on > slow paths without any lock. > > For me, you at least don't use the RCU API correctly here. > > Thanks. Yes, fast path still relies on RCU. What I meant is that slow path (cls API) now only calls tp ops after obtaining reference to tp, so there is no need to protect it from concurrent tp->ops->destroy() by means of rtnl or any other lock. I understand that using rcu_dereference_protected() is confusing in this case and will refactor this patch appropriately.
On Tue, Feb 26, 2019 at 6:57 AM Vlad Buslov <vladbu@mellanox.com> wrote: > > > On Mon 25 Feb 2019 at 22:39, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Feb 25, 2019 at 8:11 AM Vlad Buslov <vladbu@mellanox.com> wrote: > >> > >> > >> On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote: > >> > > >> > So if it is no longer RCU any more, why do you still use > >> > rcu_dereference_protected()? That is, why not just deref it as a raw > >> > pointer? > > > > > > Any answer for this question? > > I decided that since there is neither possibility of concurrent pointer > assignment nor deallocation of object that it points to, most performant > solution would be using rcu_dereference_protected() which is the only > RCU dereference helper that doesn't use READ_ONCE. I now understand that > this is confusing (and most likely doesn't provide any noticeable > performance improvement anyway!) and will change this patch to use > rcu_dereference_raw() as you suggest. Yeah, please make sure sparse is happy with that. :)
On Thu 28 Feb 2019 at 00:49, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Tue, Feb 26, 2019 at 6:57 AM Vlad Buslov <vladbu@mellanox.com> wrote: >> >> >> On Mon 25 Feb 2019 at 22:39, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> > On Mon, Feb 25, 2019 at 8:11 AM Vlad Buslov <vladbu@mellanox.com> wrote: >> >> >> >> >> >> On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> > >> >> > So if it is no longer RCU any more, why do you still use >> >> > rcu_dereference_protected()? That is, why not just deref it as a raw >> >> > pointer? >> > >> > >> > Any answer for this question? >> >> I decided that since there is neither possibility of concurrent pointer >> assignment nor deallocation of object that it points to, most performant >> solution would be using rcu_dereference_protected() which is the only >> RCU dereference helper that doesn't use READ_ONCE. I now understand that >> this is confusing (and most likely doesn't provide any noticeable >> performance improvement anyway!) and will change this patch to use >> rcu_dereference_raw() as you suggest. > > Yeah, please make sure sparse is happy with that. :) I checked my flower change with sparse. It produced a lot of warnings, some of which are several years old. None in the code I changed though: CHECK net/sched/cls_flower.c net/sched/cls_flower.c:200:20: warning: cast from restricted __be16 net/sched/cls_flower.c:200:20: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:200:20: expected unsigned short [usertype] val net/sched/cls_flower.c:200:20: got restricted __be16 [usertype] dst net/sched/cls_flower.c:200:20: warning: cast from restricted __be16 net/sched/cls_flower.c:200:20: warning: cast from restricted __be16 net/sched/cls_flower.c:201:20: warning: cast from restricted __be16 net/sched/cls_flower.c:201:20: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:201:20: expected unsigned short [usertype] val net/sched/cls_flower.c:201:20: got restricted __be16 [usertype] dst net/sched/cls_flower.c:201:20: warning: cast from restricted __be16 net/sched/cls_flower.c:201:20: warning: cast from restricted __be16 net/sched/cls_flower.c:202:19: warning: cast from restricted __be16 net/sched/cls_flower.c:202:19: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:202:19: expected unsigned short [usertype] val net/sched/cls_flower.c:202:19: got restricted __be16 [usertype] dst net/sched/cls_flower.c:202:19: warning: cast from restricted __be16 net/sched/cls_flower.c:202:19: warning: cast from restricted __be16 net/sched/cls_flower.c:203:19: warning: cast from restricted __be16 net/sched/cls_flower.c:203:19: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:203:19: expected unsigned short [usertype] val net/sched/cls_flower.c:203:19: got restricted __be16 [usertype] dst net/sched/cls_flower.c:203:19: warning: cast from restricted __be16 net/sched/cls_flower.c:203:19: warning: cast from restricted __be16 net/sched/cls_flower.c:206:21: warning: cast from restricted __be16 net/sched/cls_flower.c:206:21: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:206:21: expected unsigned short [usertype] val net/sched/cls_flower.c:206:21: got restricted __be16 [usertype] dst net/sched/cls_flower.c:206:21: warning: cast from restricted __be16 net/sched/cls_flower.c:206:21: warning: cast from restricted __be16 net/sched/cls_flower.c:206:21: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:206:42: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:207:21: warning: cast from restricted __be16 net/sched/cls_flower.c:207:21: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:207:21: expected unsigned short [usertype] val net/sched/cls_flower.c:207:21: got restricted __be16 [usertype] dst net/sched/cls_flower.c:207:21: warning: cast from restricted __be16 net/sched/cls_flower.c:207:21: warning: cast from restricted __be16 net/sched/cls_flower.c:207:21: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:207:42: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:223:20: warning: cast from restricted __be16 net/sched/cls_flower.c:223:20: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:223:20: expected unsigned short [usertype] val net/sched/cls_flower.c:223:20: got restricted __be16 [usertype] src net/sched/cls_flower.c:223:20: warning: cast from restricted __be16 net/sched/cls_flower.c:223:20: warning: cast from restricted __be16 net/sched/cls_flower.c:224:20: warning: cast from restricted __be16 net/sched/cls_flower.c:224:20: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:224:20: expected unsigned short [usertype] val net/sched/cls_flower.c:224:20: got restricted __be16 [usertype] src net/sched/cls_flower.c:224:20: warning: cast from restricted __be16 net/sched/cls_flower.c:224:20: warning: cast from restricted __be16 net/sched/cls_flower.c:225:19: warning: cast from restricted __be16 net/sched/cls_flower.c:225:19: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:225:19: expected unsigned short [usertype] val net/sched/cls_flower.c:225:19: got restricted __be16 [usertype] src net/sched/cls_flower.c:225:19: warning: cast from restricted __be16 net/sched/cls_flower.c:225:19: warning: cast from restricted __be16 net/sched/cls_flower.c:226:19: warning: cast from restricted __be16 net/sched/cls_flower.c:226:19: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:226:19: expected unsigned short [usertype] val net/sched/cls_flower.c:226:19: got restricted __be16 [usertype] src net/sched/cls_flower.c:226:19: warning: cast from restricted __be16 net/sched/cls_flower.c:226:19: warning: cast from restricted __be16 net/sched/cls_flower.c:229:21: warning: cast from restricted __be16 net/sched/cls_flower.c:229:21: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:229:21: expected unsigned short [usertype] val net/sched/cls_flower.c:229:21: got restricted __be16 [usertype] src net/sched/cls_flower.c:229:21: warning: cast from restricted __be16 net/sched/cls_flower.c:229:21: warning: cast from restricted __be16 net/sched/cls_flower.c:229:21: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:229:42: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:230:21: warning: cast from restricted __be16 net/sched/cls_flower.c:230:21: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:230:21: expected unsigned short [usertype] val net/sched/cls_flower.c:230:21: got restricted __be16 [usertype] src net/sched/cls_flower.c:230:21: warning: cast from restricted __be16 net/sched/cls_flower.c:230:21: warning: cast from restricted __be16 net/sched/cls_flower.c:230:21: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:230:42: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:744:14: warning: cast from restricted __be16 net/sched/cls_flower.c:744:14: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:744:14: expected unsigned short [usertype] val net/sched/cls_flower.c:744:14: got restricted __be16 [usertype] dst net/sched/cls_flower.c:744:14: warning: cast from restricted __be16 net/sched/cls_flower.c:744:14: warning: cast from restricted __be16 net/sched/cls_flower.c:744:40: warning: cast from restricted __be16 net/sched/cls_flower.c:744:40: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:744:40: expected unsigned short [usertype] val net/sched/cls_flower.c:744:40: got restricted __be16 [usertype] dst net/sched/cls_flower.c:744:40: warning: cast from restricted __be16 net/sched/cls_flower.c:744:40: warning: cast from restricted __be16 net/sched/cls_flower.c:744:14: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:744:40: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:746:15: warning: cast from restricted __be16 net/sched/cls_flower.c:746:15: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:746:15: expected unsigned short [usertype] val net/sched/cls_flower.c:746:15: got restricted __be16 [usertype] src net/sched/cls_flower.c:746:15: warning: cast from restricted __be16 net/sched/cls_flower.c:746:15: warning: cast from restricted __be16 net/sched/cls_flower.c:746:41: warning: cast from restricted __be16 net/sched/cls_flower.c:746:41: warning: incorrect type in argument 1 (different base types) net/sched/cls_flower.c:746:41: expected unsigned short [usertype] val net/sched/cls_flower.c:746:41: got restricted __be16 [usertype] src net/sched/cls_flower.c:746:41: warning: cast from restricted __be16 net/sched/cls_flower.c:746:41: warning: cast from restricted __be16 net/sched/cls_flower.c:746:15: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:746:41: warning: restricted __be16 degrades to integer net/sched/cls_flower.c:830:15: warning: cast to restricted __be32 net/sched/cls_flower.c:830:15: warning: cast to restricted __be32 net/sched/cls_flower.c:830:15: warning: cast to restricted __be32 net/sched/cls_flower.c:830:15: warning: cast to restricted __be32 net/sched/cls_flower.c:830:15: warning: cast to restricted __be32 net/sched/cls_flower.c:830:15: warning: cast to restricted __be32 net/sched/cls_flower.c:831:16: warning: cast to restricted __be32 net/sched/cls_flower.c:831:16: warning: cast to restricted __be32 net/sched/cls_flower.c:831:16: warning: cast to restricted __be32 net/sched/cls_flower.c:831:16: warning: cast to restricted __be32 net/sched/cls_flower.c:831:16: warning: cast to restricted __be32 net/sched/cls_flower.c:831:16: warning: cast to restricted __be32 Cong, Jiri, do you want me to send patch that fixes all of these first? Regards, Vlad
On Thu, Feb 28, 2019 at 10:35 AM Vlad Buslov <vladbu@mellanox.com> wrote: > > > On Thu 28 Feb 2019 at 00:49, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Tue, Feb 26, 2019 at 6:57 AM Vlad Buslov <vladbu@mellanox.com> wrote: > >> > >> > >> On Mon 25 Feb 2019 at 22:39, Cong Wang <xiyou.wangcong@gmail.com> wrote: > >> > On Mon, Feb 25, 2019 at 8:11 AM Vlad Buslov <vladbu@mellanox.com> wrote: > >> >> > >> >> > >> >> On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote: > >> >> > > >> >> > So if it is no longer RCU any more, why do you still use > >> >> > rcu_dereference_protected()? That is, why not just deref it as a raw > >> >> > pointer? > >> > > >> > > >> > Any answer for this question? > >> > >> I decided that since there is neither possibility of concurrent pointer > >> assignment nor deallocation of object that it points to, most performant > >> solution would be using rcu_dereference_protected() which is the only > >> RCU dereference helper that doesn't use READ_ONCE. I now understand that > >> this is confusing (and most likely doesn't provide any noticeable > >> performance improvement anyway!) and will change this patch to use > >> rcu_dereference_raw() as you suggest. > > > > Yeah, please make sure sparse is happy with that. :) > > I checked my flower change with sparse. It produced a lot of warnings, > some of which are several years old. None in the code I changed though: If so, we can address this later, it is not urgent.
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 32fa3e20adc5..88d7af78ba7e 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -433,10 +433,20 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f) cls_flower.stats.lastused); } +static struct cls_fl_head *fl_head_dereference(struct tcf_proto *tp) +{ + /* Flower classifier only changes root pointer during init and destroy. + * Cls API implements reference counting for tcf_proto, so there is no + * danger of concurrent access to tp when it is being destroyed, even + * without protection provided by rtnl lock. + */ + return rcu_dereference_protected(tp->root, 1); +} + static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f, struct netlink_ext_ack *extack) { - struct cls_fl_head *head = rtnl_dereference(tp->root); + struct cls_fl_head *head = fl_head_dereference(tp); bool async = tcf_exts_get_net(&f->exts); bool last; @@ -468,7 +478,7 @@ static void fl_destroy_sleepable(struct work_struct *work) static void fl_destroy(struct tcf_proto *tp, bool rtnl_held, struct netlink_ext_ack *extack) { - struct cls_fl_head *head = rtnl_dereference(tp->root); + struct cls_fl_head *head = fl_head_dereference(tp); struct fl_flow_mask *mask, *next_mask; struct cls_fl_filter *f, *next; @@ -486,7 +496,7 @@ static void fl_destroy(struct tcf_proto *tp, bool rtnl_held, static void *fl_get(struct tcf_proto *tp, u32 handle) { - struct cls_fl_head *head = rtnl_dereference(tp->root); + struct cls_fl_head *head = fl_head_dereference(tp); return idr_find(&head->handle_idr, handle); } @@ -1304,7 +1314,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, void **arg, bool ovr, bool rtnl_held, struct netlink_ext_ack *extack) { - struct cls_fl_head *head = rtnl_dereference(tp->root); + struct cls_fl_head *head = fl_head_dereference(tp); struct cls_fl_filter *fold = *arg; struct cls_fl_filter *fnew; struct fl_flow_mask *mask; @@ -1441,7 +1451,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, static int fl_delete(struct tcf_proto *tp, void *arg, bool *last, bool rtnl_held, struct netlink_ext_ack *extack) { - struct cls_fl_head *head = rtnl_dereference(tp->root); + struct cls_fl_head *head = fl_head_dereference(tp); struct cls_fl_filter *f = arg; rhashtable_remove_fast(&f->mask->ht, &f->ht_node, @@ -1454,7 +1464,7 @@ static int fl_delete(struct tcf_proto *tp, void *arg, bool *last, static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg, bool rtnl_held) { - struct cls_fl_head *head = rtnl_dereference(tp->root); + struct cls_fl_head *head = fl_head_dereference(tp); struct cls_fl_filter *f; arg->count = arg->skip; @@ -1473,7 +1483,7 @@ static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg, static int fl_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb, void *cb_priv, struct netlink_ext_ack *extack) { - struct cls_fl_head *head = rtnl_dereference(tp->root); + struct cls_fl_head *head = fl_head_dereference(tp); struct tc_cls_flower_offload cls_flower = {}; struct tcf_block *block = tp->chain->block; struct fl_flow_mask *mask;