You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/05/14 09:38:29 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #1047: sim/tapdev: follow the tunnel MTU size
anchao opened a new pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047
## Summary
netdev/register: configurable net packet size
sim/tapdev: follow the tunnel MTU size
net: remove unnecessary spaces
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#discussion_r425528457
##########
File path: net/netdev/netdev_register.c
##########
@@ -333,6 +335,18 @@ int netdev_register(FAR struct net_driver_s *dev, enum net_lltype_e lltype)
return -EINVAL;
}
+ /* Update the package length */
+
+ if (dev->d_llhdrlen == 0)
+ {
+ dev->d_llhdrlen = llhdrlen;
+ }
+
+ if (dev->d_pktsize == 0)
+ {
+ dev->d_pktsize = pktsize;
+ }
+
Review comment:
Configure the MTU packet size in the SIM scenario is a invalid choice,
the MTU will only take effect at the link layer when configuring the tunnel bridge:
tools/simbridge.sh:
```
if [ "$2" == "on" ]; then
ip link add nuttx0 type bridge
ip addr flush dev $1
+ ip link set dev nuttx0 mtu size?
ip link set $1 master nuttx0
ip link set dev nuttx0 up
dhclient nuttx0
```
```
nuttx0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
inet 10.0.2.15 netmask 255.255.255.0 broadcast 10.0.2.255
inet6 fe80::a00:27ff:fe63:d2e7 prefixlen 64 scopeid 0x20<link>
```
If the nuttx MTU size is small than tunnel, the larger packets will do drop processing at:
net/devif/ipv4_input.c:
```
...
377 /* Check the size of the packet. If the size reported to us in d_len is
378 * smaller the size reported in the IP header, we assume that the packet
379 * has been corrupted in transit. If the size of d_len is larger than the
380 * size reported in the IP packet header, the packet has been padded and
381 * we set d_len to the correct value.
382 */
383
384 totlen = (ipv4->len[0] << 8) + ipv4->len[1];
385 if (totlen <= dev->d_len)
386 {
387 dev->d_len = totlen;
388 }
389 else
390 {
391 nwarn("WARNING: IP packet shorter than length in IP header\n");
392 goto drop;
393 }
...
```
in this situation, I think obtain the MTU size dynamically is a necessary choice.
Regarding your concerns, We will check further whether there is side effects in the mainline.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#discussion_r425379296
##########
File path: net/netdev/netdev_register.c
##########
@@ -333,6 +335,18 @@ int netdev_register(FAR struct net_driver_s *dev, enum net_lltype_e lltype)
return -EINVAL;
}
+ /* Update the package length */
+
+ if (dev->d_llhdrlen == 0)
+ {
+ dev->d_llhdrlen = llhdrlen;
+ }
+
+ if (dev->d_pktsize == 0)
+ {
+ dev->d_pktsize = pktsize;
+ }
+
Review comment:
> This could break network drivers that do not initialilze dev->d_llhdrlen and d_pktsize to zero. Did you verify that all network drivers do this?
It would be good to have an explanation why you made this change. It does not seem right, but I do not know the motivation. I guess this would only occur if you regiser the network driver twice. If that is the case we should document when and why you do that and shy the link layer protocol of only the first registration is retained.
Do you think that you could add some comments explaining what this is for? I would appreciate that.
I don't believe that there is any problem with uninitialized data in the current network drivers. There should be no problem if the driver structure instance is defined in .bss or if the driver structure instance is allocated by kmm_zalloc(). That seems to be the case always:
#!/bin/sh
DRIVERS=`grep -rl net_driver_s *`
for driver in $DRIVERS; do
echo "=== $driver"
grep net_driver_s $driver | grep kmm_ | grep alloc | grep -v kmm_zalloc
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#issuecomment-628994539
I don't understand under what conditions this logic would be invoked. There is one one: If netdev_register() were called twice with the same driver instance. That does not happen.
Have you changed the logic in some way? Do you call netdev_register() mutliple times with the same device structure? If so, why?
The logic looks wrong to me unless you can explain when that would happen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#issuecomment-629003652
> I don't understand under what conditions this logic would be invoked. There is one one: If netdev_register() were called twice with the same driver instance. That does not happen.
>
sim just support one tapdev by desgin(many global variables are used), so netdev_register is just callded once.
> Have you changed the logic in some way? Do you call netdev_register() mutliple times with the same device structure? If so, why?
The initializition flow is same as before, the real difference is:
1.Add netdriver_setmtu
2.Call netdriver_setmtu in tapdev_init to tell NuttX what mtu size is really used by tapdev
The same logic is also used by netdriver_setmacaddr too.
Why don't we use Kconfig to determine the ethernet patcket length on sim like other platform? Because the mtu(default 1500) of tapdev can be assgined when you create the net bridge(default nuttx0):
```
ip link set dev nuttx0 mtu size?
```
The keypoint is that the mtu of tapdev can be changed dynamically, the packet length from Kconfig isn't suitable for this situation.
>
> The logic looks wrong to me unless you can explain when that would happen.
Now let's look at netdriver_register logic, it always set the d_llhdrlen/d_pktsize to Kconfig value base on the link type, it work for the most case, but not good for simulator as I described before.
How to fix this problem? we follow the same approach what d_ifname has been used:
1.If d_pktsize is zero, assign the value from Kconfig to d_pktsize
2.if d_pktsize isn't zero, use this value directly
With this change, simulator can query the real mtu from net bridge and initialize d_pktsize before calling netdriver_register. The final result is both side(NuttX and host) has the same mtu size.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#discussion_r425380709
##########
File path: net/netdev/netdev_register.c
##########
@@ -333,6 +335,18 @@ int netdev_register(FAR struct net_driver_s *dev, enum net_lltype_e lltype)
return -EINVAL;
}
+ /* Update the package length */
+
+ if (dev->d_llhdrlen == 0)
+ {
+ dev->d_llhdrlen = llhdrlen;
+ }
+
+ if (dev->d_pktsize == 0)
+ {
+ dev->d_pktsize = pktsize;
+ }
+
Review comment:
If you can explain the change, I will be happy to do the PR to update the comments so that the next person will not be confused by this either.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#issuecomment-628994539
I don't understand under what conditions this logic would be invoked. There is one one: If netdev_register() were called twice with the same driver instance. That does not happen.
Have you changed the logic in some way? Do you call netdev_register() mutliple times with the same device structure? If so, why?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#discussion_r425528457
##########
File path: net/netdev/netdev_register.c
##########
@@ -333,6 +335,18 @@ int netdev_register(FAR struct net_driver_s *dev, enum net_lltype_e lltype)
return -EINVAL;
}
+ /* Update the package length */
+
+ if (dev->d_llhdrlen == 0)
+ {
+ dev->d_llhdrlen = llhdrlen;
+ }
+
+ if (dev->d_pktsize == 0)
+ {
+ dev->d_pktsize = pktsize;
+ }
+
Review comment:
Configure the MTU packet size in the SIM scenario is a invalid choice,
the MTU will only take effect at the link layer when configuring the tunnel bridge:
tools/simbridge.sh:
```
if [ "$2" == "on" ]; then
ip link add nuttx0 type bridge
ip addr flush dev $1
+ ip link set dev nuttx0 mtu size?
ip link set $1 master nuttx0
ip link set dev nuttx0 up
dhclient nuttx0
```
```
nuttx0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
inet 10.0.2.15 netmask 255.255.255.0 broadcast 10.0.2.255
inet6 fe80::a00:27ff:fe63:d2e7 prefixlen 64 scopeid 0x20<link>
```
If the nuttx MTU size is small than tunnel, the larger packets will do drop processing at:
net/devif/ipv4_input.c:
...
377 /* Check the size of the packet. If the size reported to us in d_len is
378 * smaller the size reported in the IP header, we assume that the packet
379 * has been corrupted in transit. If the size of d_len is larger than the
380 * size reported in the IP packet header, the packet has been padded and
381 * we set d_len to the correct value.
382 */
383
384 totlen = (ipv4->len[0] << 8) + ipv4->len[1];
385 if (totlen <= dev->d_len)
386 {
387 dev->d_len = totlen;
388 }
389 else
390 {
391 nwarn("WARNING: IP packet shorter than length in IP header\n");
392 goto drop;
393 }
...
in this situation, I think obtain the MTU size dynamically is a necessary choice.
Regarding your concerns, We will check further whether there is side effects in the mainline.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#discussion_r425158448
##########
File path: net/netdev/netdev_register.c
##########
@@ -333,6 +335,18 @@ int netdev_register(FAR struct net_driver_s *dev, enum net_lltype_e lltype)
return -EINVAL;
}
+ /* Update the package length */
+
+ if (dev->d_llhdrlen == 0)
+ {
+ dev->d_llhdrlen = llhdrlen;
+ }
+
+ if (dev->d_pktsize == 0)
+ {
+ dev->d_pktsize = pktsize;
+ }
+
Review comment:
This could break network drivers that do not initialilze dev->d_llhdrlen and d_pktsize to zero. Did you verify that all network drivers do this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo merged pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#issuecomment-629003652
> I don't understand under what conditions this logic would be invoked. There is one one: If netdev_register() were called twice with the same driver instance. That does not happen.
>
sim just support one tapdev by desgin(many global variables are used), so netdev_register is just callded once.
> Have you changed the logic in some way? Do you call netdev_register() mutliple times with the same device structure? If so, why?
The initializition flow is same as before, the real difference is:
1.Add netdriver_setmtu
2.Call netdriver_setmtu in tapdev_init to tell NuttX what mtu size is really used by tapdev
The same logic is also used by netdriver_setmacaddr too.
Why don't we use Kconfig to determine the ethernet patcket length on sim like other platform? Because the mtu(default 1500) of tapdev can be assgined when you create the netbridge(default nuttx0):
```
ip link set dev nuttx0 mtu size?
```
The keypoint is that the mtu of tapdev can be changed dynamically, the packet length from Kconfig isn't suitable for this situation.
>
> The logic looks wrong to me unless you can explain when that would happen.
Now let's look at netdriver_register logic, it always set the d_llhdrlen/d_pktsize base on the link type, it work for the most case, but not good for simulator as I described before.
How to fix this problem? we follow the same approach what d_ifname has been used:
1.If d_pktsize is zero, assign the value from Kconfig to d_pktsize
2.if d_pktsize isn't zero, use this value directly
With this change, simulator can query the real mtu from net bridge and initialize d_pktsize before calling netdriver_register. The final result is both side(NuttX and host) has the same mtu size.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#issuecomment-629227299
Thanks for your explanation. I added some very brief comments to #1055 that should make it clear what is going on here. I did not specifically address the simulator, only the capabilility for the network drivers to manage the MTU.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#issuecomment-629003652
> I don't understand under what conditions this logic would be invoked. There is one one: If netdev_register() were called twice with the same driver instance. That does not happen.
>
sim just support one tapdev by desgin(many global variables are used), so netdev_register is just callded once.
> Have you changed the logic in some way? Do you call netdev_register() mutliple times with the same device structure? If so, why?
The initializition flow is same as before, the real difference is:
1.Add netdriver_setmtu
2.Call netdriver_setmtu in tapdev_init to tell NuttX what mtu size is really used by tapdev
The same logic is also used by netdriver_setmacaddr too.
Why don't we use Kconfig to determine the ethernet patcket length on sim like other platform? Because the mtu(default 1500) of tapdev can be assgined when you create the net bridge(default nuttx0):
```
ip link set dev nuttx0 mtu size?
```
The keypoint is that the mtu of tapdev can be changed dynamically, the packet length from Kconfig isn't suitable for this situation.
>
> The logic looks wrong to me unless you can explain when that would happen.
Now let's look at netdriver_register logic, it always set the d_llhdrlen/d_pktsize base on the link type, it work for the most case, but not good for simulator as I described before.
How to fix this problem? we follow the same approach what d_ifname has been used:
1.If d_pktsize is zero, assign the value from Kconfig to d_pktsize
2.if d_pktsize isn't zero, use this value directly
With this change, simulator can query the real mtu from net bridge and initialize d_pktsize before calling netdriver_register. The final result is both side(NuttX and host) has the same mtu size.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1047: sim/tapdev: follow the tunnel MTU size
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1047:
URL: https://github.com/apache/incubator-nuttx/pull/1047#issuecomment-629003652
> I don't understand under what conditions this logic would be invoked. There is one one: If netdev_register() were called twice with the same driver instance. That does not happen.
>
sim just support one tapdev by desgin(many global variables are used), so netdev_register is just callded once.
> Have you changed the logic in some way? Do you call netdev_register() mutliple times with the same device structure? If so, why?
The initializition flow is same as before, the real difference is:
1.Add netdriver_setmtu
2.Call netdriver_setmtu in tapdev_init to tell NuttX what mtu size is really used by tapdev
The same logic is also used by netdriver_setmacaddr too.
Why don't we use Kconfig to determine the ethernet patcket length on sim like other platform? Because the mtu(default 1500) of tapdev can be assgined when you create the net bridge(default nuttx0):
```
ip link set dev nuttx0 mtu size?
```
The keypoint is that the mtu of tapdev can be changed dynamically, the packet length from Kconfig isn't suitable for this situation.
>
> The logic looks wrong to me unless you can explain when that would happen.
Now let's look at netdriver_register logic, it always set the d_llhdrlen/d_pktsize to Kconfig value base on the link type, it work for the most case, but not good for simulator as I described before.
How to fix this problem? we follow the same approach what d_ifname has been used:
1.If d_pktsize is zero, assign the value from Kconfig to d_pktsize
2.if d_pktsize isn't zero, use this value directly
With this change, simulator can query the real mtu from net bridge and initialize d_pktsize before calling netdriver_register, then the final result is both side(NuttX and host) has the same mtu size. Other net driver still get patcket length from Kconfig.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org