[net-next,v2,1/3] macvlan: don't update the uc and vlan list for L2 forwarding offload

Message ID 1401969663-4464-2-git-send-email-dingtianhong@huawei.com
State New
Headers show

Commit Message

Ding Tianhong June 5, 2014, 12:01 p.m.
If lowerdev supports L2 forwarding offload, no need to set mac address
to uc list and vlan list, so also don't do that when the macvlan mac address
changes.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/macvlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Vlad Yasevich June 5, 2014, 3:09 p.m. | #1
On 06/05/2014 08:01 AM, Ding Tianhong wrote:
> If lowerdev supports L2 forwarding offload, no need to set mac address
> to uc list and vlan list, so also don't do that when the macvlan mac address
> changes.

Same issue as in v1.  Please explain how this actually works, since the
new address never makes it to the hw.

-vlad
> 
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>  drivers/net/macvlan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 453d55a..c3a54a6 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
>  	struct net_device *lowerdev = vlan->lowerdev;
>  	int err;
>  
> -	if (!(dev->flags & IFF_UP)) {
> +	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
>  		/* Just copy in the new address */
>  		ether_addr_copy(dev->dev_addr, addr);
>  	} else {
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ding Tianhong June 6, 2014, 1:42 a.m. | #2
On 2014/6/5 23:09, Vlad Yasevich wrote:
> On 06/05/2014 08:01 AM, Ding Tianhong wrote:
>> If lowerdev supports L2 forwarding offload, no need to set mac address
>> to uc list and vlan list, so also don't do that when the macvlan mac address
>> changes.
> 
> Same issue as in v1.  Please explain how this actually works, since the
> new address never makes it to the hw.
> 
> -vlad
>>

Sorry, I miss the first feedback of yours, so didn't fix the issue from v1, I will think about it and than answer it, thanks.

Ding

>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>>  drivers/net/macvlan.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>> index 453d55a..c3a54a6 100644
>> --- a/drivers/net/macvlan.c
>> +++ b/drivers/net/macvlan.c
>> @@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
>>  	struct net_device *lowerdev = vlan->lowerdev;
>>  	int err;
>>  
>> -	if (!(dev->flags & IFF_UP)) {
>> +	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
>>  		/* Just copy in the new address */
>>  		ether_addr_copy(dev->dev_addr, addr);
>>  	} else {
>>
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ding Tianhong June 6, 2014, 3:45 a.m. | #3
On 2014/6/5 23:09, Vlad Yasevich wrote:
> On 06/05/2014 08:01 AM, Ding Tianhong wrote:
>> If lowerdev supports L2 forwarding offload, no need to set mac address
>> to uc list and vlan list, so also don't do that when the macvlan mac address
>> changes.
> 
> Same issue as in v1.  Please explain how this actually works, since the
> new address never makes it to the hw.
> 

Hi, vlad:

The fwd only support 82599 nic, and when fwd_priv is true, this means the lowerdev support L2 forwarding offload, and will set the macvlan
to the ixgbe_fwd_adapter->netdev, and then it is clear that the lowerdev could know that the the upper device is macvlan and could handle
the skb to the upperdev.

If I miss something, please remind me, thanks.

Ding

> -vlad
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>>  drivers/net/macvlan.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>> index 453d55a..c3a54a6 100644
>> --- a/drivers/net/macvlan.c
>> +++ b/drivers/net/macvlan.c
>> @@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
>>  	struct net_device *lowerdev = vlan->lowerdev;
>>  	int err;
>>  
>> -	if (!(dev->flags & IFF_UP)) {
>> +	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
>>  		/* Just copy in the new address */
>>  		ether_addr_copy(dev->dev_addr, addr);
>>  	} else {
>>
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich June 6, 2014, 5:11 p.m. | #4
On 06/05/2014 11:45 PM, Ding Tianhong wrote:
> On 2014/6/5 23:09, Vlad Yasevich wrote:
>> On 06/05/2014 08:01 AM, Ding Tianhong wrote:
>>> If lowerdev supports L2 forwarding offload, no need to set mac address
>>> to uc list and vlan list, so also don't do that when the macvlan mac address
>>> changes.
>>
>> Same issue as in v1.  Please explain how this actually works, since the
>> new address never makes it to the hw.
>>
> 
> Hi, vlad:
> 
> The fwd only support 82599 nic, and when fwd_priv is true, this means the lowerdev support L2 forwarding offload, and will set the macvlan
> to the ixgbe_fwd_adapter->netdev, and then it is clear that the lowerdev could know that the the upper device is macvlan and could handle
> the skb to the upperdev.
> 
> If I miss something, please remind me, thanks.

You've just changed the mac address of the macvlan device, but the
address is not written to the card's RAR registers.
So what allows the card to receive the traffic and place it into the
queue in the first place?

I can see it possibly working if the card is in promiscuous mode.

Also, I think John is right in that if we simply do dev_uc_add()
to add the address to the card, we'll unmap the address from the vmdqs
and lose the acceleration.  This needs more thought.

-vlad

> 
> Ding
> 
>> -vlad
>>>
>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>> ---
>>>  drivers/net/macvlan.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>>> index 453d55a..c3a54a6 100644
>>> --- a/drivers/net/macvlan.c
>>> +++ b/drivers/net/macvlan.c
>>> @@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
>>>  	struct net_device *lowerdev = vlan->lowerdev;
>>>  	int err;
>>>  
>>> -	if (!(dev->flags & IFF_UP)) {
>>> +	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
>>>  		/* Just copy in the new address */
>>>  		ether_addr_copy(dev->dev_addr, addr);
>>>  	} else {
>>>
>>
>>
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ding Tianhong June 7, 2014, 5:59 a.m. | #5
On 2014/6/7 1:11, Vlad Yasevich wrote:
> On 06/05/2014 11:45 PM, Ding Tianhong wrote:
>> On 2014/6/5 23:09, Vlad Yasevich wrote:
>>> On 06/05/2014 08:01 AM, Ding Tianhong wrote:
>>>> If lowerdev supports L2 forwarding offload, no need to set mac address
>>>> to uc list and vlan list, so also don't do that when the macvlan mac address
>>>> changes.
>>>
>>> Same issue as in v1.  Please explain how this actually works, since the
>>> new address never makes it to the hw.
>>>
>>
>> Hi, vlad:
>>
>> The fwd only support 82599 nic, and when fwd_priv is true, this means the lowerdev support L2 forwarding offload, and will set the macvlan
>> to the ixgbe_fwd_adapter->netdev, and then it is clear that the lowerdev could know that the the upper device is macvlan and could handle
>> the skb to the upperdev.
>>
>> If I miss something, please remind me, thanks.
> 
> You've just changed the mac address of the macvlan device, but the
> address is not written to the card's RAR registers.
> So what allows the card to receive the traffic and place it into the
> queue in the first place?
> 
> I can see it possibly working if the card is in promiscuous mode.
> 
> Also, I think John is right in that if we simply do dev_uc_add()
> to add the address to the card, we'll unmap the address from the vmdqs
> and lose the acceleration.  This needs more thought.
> 

Yes, I miss it, the new mac address should be set to the lowerdev by ndo_dfwd_add_station(),
otherwise the lowerdev still use the old mac address and the macvlan could not work again,
so I need to rethink this patch and resend, thanks, vlad.

Ding

> -vlad
> 
>>
>> Ding
>>
>>> -vlad
>>>>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>>> ---
>>>>  drivers/net/macvlan.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>>>> index 453d55a..c3a54a6 100644
>>>> --- a/drivers/net/macvlan.c
>>>> +++ b/drivers/net/macvlan.c
>>>> @@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
>>>>  	struct net_device *lowerdev = vlan->lowerdev;
>>>>  	int err;
>>>>  
>>>> -	if (!(dev->flags & IFF_UP)) {
>>>> +	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
>>>>  		/* Just copy in the new address */
>>>>  		ether_addr_copy(dev->dev_addr, addr);
>>>>  	} else {
>>>>
>>>
>>>
>>>
>>
>>
> 
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 453d55a..c3a54a6 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -515,7 +515,7 @@  static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
-	if (!(dev->flags & IFF_UP)) {
+	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
 		/* Just copy in the new address */
 		ether_addr_copy(dev->dev_addr, addr);
 	} else {