On Thu, Nov 2, 2017 at 5:07 AM, Pravin Shelar <pshe...@ovn.org> wrote:
> On Thu, Nov 2, 2017 at 3:07 AM, Andy Zhou <az...@ovn.org> wrote:
>> On Fri, Oct 20, 2017 at 8:32 PM, Pravin Shelar <pshe...@ovn.org> wrote:
>>> On Thu, Oct 19, 2017 at 5:58 PM, Andy Zhou <az...@ovn.org> wrote:
>>>>
>>>> On Thu, Oct 19, 2017 at 02:47 Pravin Shelar <pshe...@ovn.org> wrote:
>>>>>
>>>>> On Tue, Oct 17, 2017 at 12:36 AM, Andy Zhou <az...@ovn.org> wrote:
>>>>> > OVS kernel datapath so far does not support Openflow meter action.
>>>>> > This is the first stab at adding kernel datapath meter support.
>>>>> > This implementation supports only drop band type.
>>>>> >
>>>>> > Signed-off-by: Andy Zhou <az...@ovn.org>
>>>>> > ---
>>>>> > net/openvswitch/Makefile | 1 +
>>>>> > net/openvswitch/datapath.c | 14 +-
>>>>> > net/openvswitch/datapath.h | 3 +
>>>>> > net/openvswitch/meter.c | 604
>>>>> > +++++++++++++++++++++++++++++++++++++++++++++
>>>>> > net/openvswitch/meter.h | 54 ++++
>>>>> > 5 files changed, 674 insertions(+), 2 deletions(-)
>>>>> > create mode 100644 net/openvswitch/meter.c
>>>>> > create mode 100644 net/openvswitch/meter.h
>>>>> >
>>>>> This patch mostly looks good. I have one comment below.
>>>>>
>>>>> > +static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info
>>>>> > *info)
>>>>> > +{
>>>>> > + struct nlattr **a = info->attrs;
>>>>> > + struct dp_meter *meter, *old_meter;
>>>>> > + struct sk_buff *reply;
>>>>> > + struct ovs_header *ovs_reply_header;
>>>>> > + struct ovs_header *ovs_header = info->userhdr;
>>>>> > + struct datapath *dp;
>>>>> > + int err;
>>>>> > + u32 meter_id;
>>>>> > + bool failed;
>>>>> > +
>>>>> > + meter = dp_meter_create(a);
>>>>> > + if (IS_ERR_OR_NULL(meter))
>>>>> > + return PTR_ERR(meter);
>>>>> > +
>>>>> > + reply = ovs_meter_cmd_reply_start(info, OVS_METER_CMD_SET,
>>>>> > + &ovs_reply_header);
>>>>> > + if (IS_ERR(reply)) {
>>>>> > + err = PTR_ERR(reply);
>>>>> > + goto exit_free_meter;
>>>>> > + }
>>>>> > +
>>>>> > + ovs_lock();
>>>>> > + dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>>>>> > + if (!dp) {
>>>>> > + err = -ENODEV;
>>>>> > + goto exit_unlock;
>>>>> > + }
>>>>> > +
>>>>> > + if (!a[OVS_METER_ATTR_ID]) {
>>>>> > + err = -ENODEV;
>>>>> > + goto exit_unlock;
>>>>> > + }
>>>>> > +
>>>>> > + meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
>>>>> > +
>>>>> > + /* Cannot fail after this. */
>>>>> > + old_meter = lookup_meter(dp, meter_id);
>>>>> I do not see RCU read lock taken here. This is not correctness issue
>>>>> but it could cause RCU checker to spit out warning message. You could
>>>>> do same trick that is done in get_dp() to avoid this issue.
>>>>
>>>> O.K.
>>>>>
>>>>>
>>>>>
>>>>> Can you also test the code with rcu sparse check config option enabled?
>>>>
>>>>
>>>> Do you mean to sparse compile with CONFIG_PROVE_LOCKING and
>>>> CONFIG_DENUG_OBJECTS_RCU_HEAD?
>>>
>>> You could use all following options simultaneously:
>>> CONFIG_PREEMPT
>>> CONFIG_DEBUG_PREEMPT
>>> CONFIG_DEBUG_SPINLOCK
>>> CONFIG_DEBUG_ATOMIC_SLEEP
>>> CONFIG_PROVE_RCU
>>> CONFIG_DEBUG_OBJECTS_RCU_HEAD
>>
>> Thanks, I turned on those flags but did not get any error message. Do you
>> mind share the RCU checker message?
>
> There would be assert failure and stack trace. so it would be pretty
> obvious in kernel log messages.
> Let me know if you do not see any stack trace while running meter
> create, delete and execute.
No I did not see them.
