[PATCH net] openvswitch: restore OVS_FLOW_CMD_NEW notifications

[PATCH net] openvswitch: restore OVS_FLOW_CMD_NEW notifications

[Nicolas Dichtel]

From: Samuel Gauthier <samuel.gauthier@6wind.com>

Since commit fb5d1e9e127a ("openvswitch: Build flow cmd netlink reply only if needed."),
the new flows are not notified to the listeners of OVS_FLOW_MCGROUP.

This commit fixes the problem by checking that there are listeners in
the actual OVS_FLOW_MCGROUP group, instead of 0.

Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/openvswitch/datapath.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 91d66b7e64ac..8396f6063343 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -79,10 +79,10 @@ static const struct genl_multicast_group ovs_dp_vport_multicast_group = {
 /* Check if need to build a reply message.
  * OVS userspace sets the NLM_F_ECHO flag if it needs the reply. */
 static bool ovs_must_notify(struct genl_info *info,
-			    const struct genl_multicast_group *grp)
+			    unsigned int group)
 {
 	return info->nlhdr->nlmsg_flags & NLM_F_ECHO ||
-		netlink_has_listeners(genl_info_net(info)->genl_sock, 0);
+		netlink_has_listeners(genl_info_net(info)->genl_sock, group);
 }
 
 static void ovs_notify(struct genl_family *family,
@@ -763,7 +763,7 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *act
 {
 	struct sk_buff *skb;
 
-	if (!always && !ovs_must_notify(info, &ovs_dp_flow_multicast_group))
+	if (!always && !ovs_must_notify(info, dp_flow_genl_family.mcgrp_offset))
 		return NULL;
 
 	skb = genlmsg_new_unicast(ovs_flow_cmd_msg_size(acts), info, GFP_KERNEL);
-- 
2.1.0

Re: [PATCH net] openvswitch: restore OVS_FLOW_CMD_NEW notifications

[Pravin Shelar]

On Wed, Sep 17, 2014 at 9:13 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> From: Samuel Gauthier <samuel.gauthier@6wind.com>
>
> Since commit fb5d1e9e127a ("openvswitch: Build flow cmd netlink reply only if needed."),
> the new flows are not notified to the listeners of OVS_FLOW_MCGROUP.
>
> This commit fixes the problem by checking that there are listeners in
> the actual OVS_FLOW_MCGROUP group, instead of 0.
>

right, this is bug. group id should be one.

> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/openvswitch/datapath.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 91d66b7e64ac..8396f6063343 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -79,10 +79,10 @@ static const struct genl_multicast_group ovs_dp_vport_multicast_group = {
>  /* Check if need to build a reply message.
>   * OVS userspace sets the NLM_F_ECHO flag if it needs the reply. */
>  static bool ovs_must_notify(struct genl_info *info,
> -                           const struct genl_multicast_group *grp)
> +                           unsigned int group)
>  {
>         return info->nlhdr->nlmsg_flags & NLM_F_ECHO ||
> -               netlink_has_listeners(genl_info_net(info)->genl_sock, 0);
> +               netlink_has_listeners(genl_info_net(info)->genl_sock, group);
>  }
>
>  static void ovs_notify(struct genl_family *family,
> @@ -763,7 +763,7 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *act
>  {
>         struct sk_buff *skb;
>
> -       if (!always && !ovs_must_notify(info, &ovs_dp_flow_multicast_group))
> +       if (!always && !ovs_must_notify(info, dp_flow_genl_family.mcgrp_offset))
>                 return NULL;
>


I do not think mcgrp_offset is right group id. It is starting offset
of groups in a family and it is marked as private member of genl
module.
Rather we can just define enum for ovs_dp_flow_multicast_group and
directly use that as group-id.


>         skb = genlmsg_new_unicast(ovs_flow_cmd_msg_size(acts), info, GFP_KERNEL);
> --
> 2.1.0
>

Re: [PATCH net] openvswitch: restore OVS_FLOW_CMD_NEW notifications

[Pravin Shelar]

On Wed, Sep 17, 2014 at 2:56 PM, Pravin Shelar <pshelar@nicira.com> wrote:
> On Wed, Sep 17, 2014 at 9:13 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>> From: Samuel Gauthier <samuel.gauthier@6wind.com>
>>
>> Since commit fb5d1e9e127a ("openvswitch: Build flow cmd netlink reply only if needed."),
>> the new flows are not notified to the listeners of OVS_FLOW_MCGROUP.
>>
>> This commit fixes the problem by checking that there are listeners in
>> the actual OVS_FLOW_MCGROUP group, instead of 0.
>>
>
> right, this is bug. group id should be one.
>
>> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>  net/openvswitch/datapath.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
>> index 91d66b7e64ac..8396f6063343 100644
>> --- a/net/openvswitch/datapath.c
>> +++ b/net/openvswitch/datapath.c
>> @@ -79,10 +79,10 @@ static const struct genl_multicast_group ovs_dp_vport_multicast_group = {
>>  /* Check if need to build a reply message.
>>   * OVS userspace sets the NLM_F_ECHO flag if it needs the reply. */
>>  static bool ovs_must_notify(struct genl_info *info,
>> -                           const struct genl_multicast_group *grp)
>> +                           unsigned int group)
>>  {
>>         return info->nlhdr->nlmsg_flags & NLM_F_ECHO ||
>> -               netlink_has_listeners(genl_info_net(info)->genl_sock, 0);
>> +               netlink_has_listeners(genl_info_net(info)->genl_sock, group);
>>  }
>>
>>  static void ovs_notify(struct genl_family *family,
>> @@ -763,7 +763,7 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *act
>>  {
>>         struct sk_buff *skb;
>>
>> -       if (!always && !ovs_must_notify(info, &ovs_dp_flow_multicast_group))
>> +       if (!always && !ovs_must_notify(info, dp_flow_genl_family.mcgrp_offset))
>>                 return NULL;
>>
>
>
> I do not think mcgrp_offset is right group id. It is starting offset
> of groups in a family and it is marked as private member of genl
> module.
> Rather we can just define enum for ovs_dp_flow_multicast_group and
> directly use that as group-id.
>

I just realized that there is no equivalent netlink_has_listeners()
function in genl, so we have to add such function first.

[PATCH net v2 1/2] genetlink: add function genl_has_listeners()

[Nicolas Dichtel]

This function is the counterpart of the function netlink_has_listeners().

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v2: add patch 1/2

 include/net/genetlink.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 93695f0e22a5..af10c2cf8a1d 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -394,4 +394,12 @@ static inline int genl_set_err(struct genl_family *family, struct net *net,
 	return netlink_set_err(net->genl_sock, portid, group, code);
 }
 
+static inline int genl_has_listeners(struct genl_family *family,
+				     struct sock *sk, unsigned int group)
+{
+	if (WARN_ON_ONCE(group >= family->n_mcgrps))
+		return -EINVAL;
+	group = family->mcgrp_offset + group;
+	return netlink_has_listeners(sk, group);
+}
 #endif	/* __NET_GENERIC_NETLINK_H */
-- 
2.1.0

[PATCH net v2 2/2] openvswitch: restore OVS_FLOW_CMD_NEW notifications

[Nicolas Dichtel]

From: Samuel Gauthier <samuel.gauthier@6wind.com>

Since commit fb5d1e9e127a ("openvswitch: Build flow cmd netlink reply only if needed."),
the new flows are not notified to the listeners of OVS_FLOW_MCGROUP.

This commit fixes the problem by using the genl function, ie
genl_has_listerners() instead of netlink_has_listeners().

Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v2: add patch 1/2

 net/openvswitch/datapath.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 91d66b7e64ac..64dc864a417f 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -78,11 +78,12 @@ static const struct genl_multicast_group ovs_dp_vport_multicast_group = {
 
 /* Check if need to build a reply message.
  * OVS userspace sets the NLM_F_ECHO flag if it needs the reply. */
-static bool ovs_must_notify(struct genl_info *info,
-			    const struct genl_multicast_group *grp)
+static bool ovs_must_notify(struct genl_family *family, struct genl_info *info,
+			    unsigned int group)
 {
 	return info->nlhdr->nlmsg_flags & NLM_F_ECHO ||
-		netlink_has_listeners(genl_info_net(info)->genl_sock, 0);
+	       genl_has_listeners(family, genl_info_net(info)->genl_sock,
+				  group);
 }
 
 static void ovs_notify(struct genl_family *family,
@@ -763,7 +764,7 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *act
 {
 	struct sk_buff *skb;
 
-	if (!always && !ovs_must_notify(info, &ovs_dp_flow_multicast_group))
+	if (!always && !ovs_must_notify(&dp_flow_genl_family, info, 0))
 		return NULL;
 
 	skb = genlmsg_new_unicast(ovs_flow_cmd_msg_size(acts), info, GFP_KERNEL);
-- 
2.1.0

Re: [PATCH net v2 1/2] genetlink: add function genl_has_listeners()

[Pravin Shelar]

On Thu, Sep 18, 2014 at 1:31 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> This function is the counterpart of the function netlink_has_listeners().
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Looks good.
Acked-by: Pravin B Shelar <pshelar@nicira.com>

> ---
>
> v2: add patch 1/2
>
>  include/net/genetlink.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index 93695f0e22a5..af10c2cf8a1d 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -394,4 +394,12 @@ static inline int genl_set_err(struct genl_family *family, struct net *net,
>         return netlink_set_err(net->genl_sock, portid, group, code);
>  }
>
> +static inline int genl_has_listeners(struct genl_family *family,
> +                                    struct sock *sk, unsigned int group)
> +{
> +       if (WARN_ON_ONCE(group >= family->n_mcgrps))
> +               return -EINVAL;
> +       group = family->mcgrp_offset + group;
> +       return netlink_has_listeners(sk, group);
> +}
>  #endif /* __NET_GENERIC_NETLINK_H */
> --
> 2.1.0
>

Re: [PATCH net v2 2/2] openvswitch: restore OVS_FLOW_CMD_NEW notifications

[Pravin Shelar]

On Thu, Sep 18, 2014 at 1:31 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> From: Samuel Gauthier <samuel.gauthier@6wind.com>
>
> Since commit fb5d1e9e127a ("openvswitch: Build flow cmd netlink reply only if needed."),
> the new flows are not notified to the listeners of OVS_FLOW_MCGROUP.
>
> This commit fixes the problem by using the genl function, ie
> genl_has_listerners() instead of netlink_has_listeners().
>
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Thanks for the fix.
Acked-by: Pravin B Shelar <pshelar@nicira.com>

> ---
>
> v2: add patch 1/2
>
>  net/openvswitch/datapath.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 91d66b7e64ac..64dc864a417f 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -78,11 +78,12 @@ static const struct genl_multicast_group ovs_dp_vport_multicast_group = {
>
>  /* Check if need to build a reply message.
>   * OVS userspace sets the NLM_F_ECHO flag if it needs the reply. */
> -static bool ovs_must_notify(struct genl_info *info,
> -                           const struct genl_multicast_group *grp)
> +static bool ovs_must_notify(struct genl_family *family, struct genl_info *info,
> +                           unsigned int group)
>  {
>         return info->nlhdr->nlmsg_flags & NLM_F_ECHO ||
> -               netlink_has_listeners(genl_info_net(info)->genl_sock, 0);
> +              genl_has_listeners(family, genl_info_net(info)->genl_sock,
> +                                 group);
>  }
>
>  static void ovs_notify(struct genl_family *family,
> @@ -763,7 +764,7 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *act
>  {
>         struct sk_buff *skb;
>
> -       if (!always && !ovs_must_notify(info, &ovs_dp_flow_multicast_group))
> +       if (!always && !ovs_must_notify(&dp_flow_genl_family, info, 0))
>                 return NULL;
>
>         skb = genlmsg_new_unicast(ovs_flow_cmd_msg_size(acts), info, GFP_KERNEL);
> --
> 2.1.0
>

Re: [PATCH net v2 1/2] genetlink: add function genl_has_listeners()

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 18 Sep 2014 10:31:03 +0200

> This function is the counterpart of the function netlink_has_listeners().
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied.

Re: [PATCH net v2 2/2] openvswitch: restore OVS_FLOW_CMD_NEW notifications

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 18 Sep 2014 10:31:04 +0200

> From: Samuel Gauthier <samuel.gauthier@6wind.com>
> 
> Since commit fb5d1e9e127a ("openvswitch: Build flow cmd netlink reply only if needed."),
> the new flows are not notified to the listeners of OVS_FLOW_MCGROUP.
> 
> This commit fixes the problem by using the genl function, ie
> genl_has_listerners() instead of netlink_has_listeners().
> 
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Also applied, thanks.