[PATCH net-next 4/6] bonding: don't validate arp if we don't have to

[PATCH net-next 4/6] bonding: don't validate arp if we don't have to

[Veaceslav Falico]

Currently, we validate all the incoming arps if arp_validate not 0.
However, we don't have to validate backup slaves if arp_validate == active
and vice versa, so return early in bond_arp_rcv() in these cases.

It works correctly now because we verify arp_validate in slave_last_rx(),
however we're just doing useless work in bond_arp_rcv().

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b69c7f0..2cfbb2e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2624,6 +2624,10 @@ static int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
 		return RX_HANDLER_ANOTHER;
 
 	read_lock(&bond->lock);
+
+	if (!slave_do_arp_validate(bond, slave))
+		goto out_unlock;
+
 	alen = arp_hdr_len(bond->dev);
 
 	pr_debug("bond_arp_rcv: bond %s skb->dev %s\n",
-- 
1.7.1

Re: [PATCH net-next 4/6] bonding: don't validate arp if we don't have to

[Nikolay Aleksandrov]

On 19/06/13 19:34, Veaceslav Falico wrote:
> Currently, we validate all the incoming arps if arp_validate not 0.
> However, we don't have to validate backup slaves if arp_validate == active
> and vice versa, so return early in bond_arp_rcv() in these cases.
> 
> It works correctly now because we verify arp_validate in slave_last_rx(),
> however we're just doing useless work in bond_arp_rcv().
> 
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>  drivers/net/bonding/bond_main.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index b69c7f0..2cfbb2e 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2624,6 +2624,10 @@ static int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
>  		return RX_HANDLER_ANOTHER;
>  
>  	read_lock(&bond->lock);
> +
> +	if (!slave_do_arp_validate(bond, slave))
> +		goto out_unlock;
> +
>  	alen = arp_hdr_len(bond->dev);
>  
>  	pr_debug("bond_arp_rcv: bond %s skb->dev %s\n",
Hm, I think this issue runs deeper because recv_probe can be wrong and
also if arp_validate is enabled while the bond is running then
recv_probe is not set (or unset for that matter if disabled). I have a
patch which needs little more work for some time now in my queue that
fixes this, but if you'd like to fix it I'd suggest addressing that
issue (recv_probe), because then you can just drop these checks and
improve performance when disabled (after it's been enabled).
This got a bit confusing when I read it :-)

Cheers,
 Nik

Re: [PATCH net-next 4/6] bonding: don't validate arp if we don't have to

[Veaceslav Falico]

On Thu, Jun 20, 2013 at 12:19:04AM +0200, Nikolay Aleksandrov wrote:
>On 19/06/13 19:34, Veaceslav Falico wrote:
>> Currently, we validate all the incoming arps if arp_validate not 0.
>> However, we don't have to validate backup slaves if arp_validate == active
>> and vice versa, so return early in bond_arp_rcv() in these cases.
>>
>> It works correctly now because we verify arp_validate in slave_last_rx(),
>> however we're just doing useless work in bond_arp_rcv().
>>
>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>> ---
>>  drivers/net/bonding/bond_main.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index b69c7f0..2cfbb2e 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2624,6 +2624,10 @@ static int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
>>  		return RX_HANDLER_ANOTHER;
>>
>>  	read_lock(&bond->lock);
>> +
>> +	if (!slave_do_arp_validate(bond, slave))
>> +		goto out_unlock;
>> +
>>  	alen = arp_hdr_len(bond->dev);
>>
>>  	pr_debug("bond_arp_rcv: bond %s skb->dev %s\n",
>Hm, I think this issue runs deeper because recv_probe can be wrong and
>also if arp_validate is enabled while the bond is running then
>recv_probe is not set (or unset for that matter if disabled). I have a
>patch which needs little more work for some time now in my queue that
>fixes this, but if you'd like to fix it I'd suggest addressing that
>issue (recv_probe), because then you can just drop these checks and
>improve performance when disabled (after it's been enabled).

Yup, recv_probe value is really poorly synced with the arp_validate, I'll
try to take a look at it when I have time and in case you won't fix it by
that time :).

However, I don't think we should drop this check even in this case. This
check just verifies if we should validate this exact slave - being it
active or backup, and considering the value of arp_validate (which can be
active/backup/both).

Maybe I've understood you wrong, though :).

>This got a bit confusing when I read it :-)
>
>Cheers,
> Nik

Re: [PATCH net-next 4/6] bonding: don't validate arp if we don't have to

[Nikolay Aleksandrov]

On 06/20/2013 10:43 AM, Veaceslav Falico wrote:
> On Thu, Jun 20, 2013 at 12:19:04AM +0200, Nikolay Aleksandrov wrote:
>> On 19/06/13 19:34, Veaceslav Falico wrote:
>>> Currently, we validate all the incoming arps if arp_validate not 0.
>>> However, we don't have to validate backup slaves if arp_validate == active
>>> and vice versa, so return early in bond_arp_rcv() in these cases.
>>>
>>> It works correctly now because we verify arp_validate in slave_last_rx(),
>>> however we're just doing useless work in bond_arp_rcv().
>>>
>>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>>> ---
>>>  drivers/net/bonding/bond_main.c |    4 ++++
>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index b69c7f0..2cfbb2e 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -2624,6 +2624,10 @@ static int bond_arp_rcv(const struct sk_buff *skb,
>>> struct bonding *bond,
>>>          return RX_HANDLER_ANOTHER;
>>>
>>>      read_lock(&bond->lock);
>>> +
>>> +    if (!slave_do_arp_validate(bond, slave))
>>> +        goto out_unlock;
>>> +
>>>      alen = arp_hdr_len(bond->dev);
>>>
>>>      pr_debug("bond_arp_rcv: bond %s skb->dev %s\n",
>> Hm, I think this issue runs deeper because recv_probe can be wrong and
>> also if arp_validate is enabled while the bond is running then
>> recv_probe is not set (or unset for that matter if disabled). I have a
>> patch which needs little more work for some time now in my queue that
>> fixes this, but if you'd like to fix it I'd suggest addressing that
>> issue (recv_probe), because then you can just drop these checks and
>> improve performance when disabled (after it's been enabled).
> 
> Yup, recv_probe value is really poorly synced with the arp_validate, I'll
> try to take a look at it when I have time and in case you won't fix it by
> that time :).
> 
> However, I don't think we should drop this check even in this case. This
> check just verifies if we should validate this exact slave - being it
> active or backup, and considering the value of arp_validate (which can be
> active/backup/both).
> 
> Maybe I've understood you wrong, though :).
> 
>> This got a bit confusing when I read it :-)
>>
>> Cheers,
>> Nik
I agree with this patch, I didn't mean to drop it :-) My intention was more to
augment it to include the fix for recv_probe as well. But that is not critical
so it can be done at a later time.

Cheers,
 Nik