On Fri, Jul 19, 2024 at 09:02:29AM +0800, Jason Wang wrote: > On Wed, Jul 17, 2024 at 2:53 PM Jason Wang <jasowang@xxxxxxxxxx> wrote: > > > > On Wed, Jul 17, 2024 at 2:00 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote: > > > > > > On Wed, Jul 17, 2024 at 09:19:02AM +0800, Jason Wang wrote: > > > > On Wed, Jul 10, 2024 at 11:03 AM Jason Wang <jasowang@xxxxxxxxxx> wrote: > > > > > > > > > > On Tue, Jul 9, 2024 at 9:28 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote: > > > > > > > > > > > > On Tue, Jul 09, 2024 at 04:02:14PM +0800, Jason Wang wrote: > > > > > > > This patch synchronize operstate with admin state per RFC2863. > > > > > > > > > > > > > > This is done by trying to toggle the carrier upon open/close and > > > > > > > synchronize with the config change work. This allows propagate status > > > > > > > correctly to stacked devices like: > > > > > > > > > > > > > > ip link add link enp0s3 macvlan0 type macvlan > > > > > > > ip link set link enp0s3 down > > > > > > > ip link show > > > > > > > > > > > > > > Before this patch: > > > > > > > > > > > > > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000 > > > > > > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff > > > > > > > ...... > > > > > > > 5: macvlan0@enp0s3: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000 > > > > > > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff > > > > > > > > > > > > > > After this patch: > > > > > > > > > > > > > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000 > > > > > > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff > > > > > > > ... > > > > > > > 5: macvlan0@enp0s3: <NO-CARRIER,BROADCAST,MULTICAST,UP,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000 > > > > > > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff > > > > > > > > > > > > I think that the commit log is confusing. It seems to say that > > > > > > the issue fixed is synchronizing state with hardware > > > > > > config change. > > > > > > But your example does not show any > > > > > > hardware change. Isn't this example really just > > > > > > a side effect of setting carrier off on close? > > > > > > > > > > The main goal for this patch is to make virtio-net follow RFC2863. The > > > > > main thing that is missed is to synchronize the operstate with admin > > > > > state, if we do this, we get several good results, one of the obvious > > > > > one is to allow virtio-net to propagate status to the upper layer, for > > > > > example if the admin state of the lower virtio-net is down it should > > > > > be propagated to the macvlan on top, so I give the example of using a > > > > > stacked device. I'm not we had others but the commit log is probably > > > > > too small to say all of it. > > > > > > > > Michael, any more comments on this? > > > > > > > > Thans > > > > > > > > > Still don't get it, sorry. > > > > > > > This is done by trying to toggle the carrier upon open/close and > > > > > > > synchronize with the config change work. > > > What does this sentence mean? What is not synchronized with config > > > change that needs to be? > > > > I meant, > > > > 1) maclvan depends on the linkwatch to transfer operstate from the > > lower device to itself. > > 2) ndo_open()/close() will not trigger the linkwatch so we need to do > > it by ourselves in virtio-net to make sure macvlan get the correct > > opersate > > 3) consider config change work can change the state so ndo_close() > > needs to synchronize with it > > > > Thanks > > Michael, are you fine with the above or I miss something there? > > Thanks I don't understand 3. config change can always trigger. what I do not like is all these reads from config space that now trigger on open/close. previously we did read - on probe - after probe, if config changed and that made sense. -- MST
