| Message ID | 20240413084826.52417-1-jun.gu@easystack.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] net: openvswitch: Check vport name | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 13 Apr 2024, at 10:48, jun.gu wrote: > Check vport name from dev_get_by_name, this can avoid to add and remove > NIC repeatedly when NIC rename failed at system startup. > > Signed-off-by: Jun Gu <jun.gu@easystack.cn> > --- > net/openvswitch/vport-netdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c > index 903537a5da22..de8977d7f329 100644 > --- a/net/openvswitch/vport-netdev.c > +++ b/net/openvswitch/vport-netdev.c > @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name) > int err; > > vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name); > - if (!vport->dev) { > + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) { Hi Jun, not sure if I get the point here, as ovs_vport_name() translates into vport->dev->name. So are we trying to catch the interface rename between the dev_get_by_name(), and the code below? This rename could happen at any other place, so this check does not guarantee anything. Or am I missing something? > err = -ENODEV; > goto error_free_vport; > } > -- > 2.25.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Chaudron, thanks for your reply. Considerthe following scenario:
- set a net device alias name (such as *enpxx*) into ovs.
- |OVS_VPORT_CMD_NEW -> ||dev_get_by_name can query the net device using
*enpxx* name, but the dev->name that return is *ensxx* name. the |net_device|struct including: ``` |struct net_device {
char name[IFNAMSIZ];
RH_KABI_REPLACE(struct hlist_node name_hlist,
struct netdev_name_node *name_node)
...
|``` name_hlist include enpxx and ensxx name and both of them point to
the same net_device, the net_device's name is ensxx. So when using
*enpxx* name to query net device, the ||net device's name that return is
*ensxx* name.|
Then, openvswitch.ko will save the net device that name is*ensxx* to a hash table. So this will cause the below process:
- ovs can'tobtain the enpxx net device by |OVS_VPORT_CMD_GET.|
-|dpif_netlink_port_poll will get a |notification after |OVS_VPORT_CMD_NEW|, but the vport's name is ensxx. ovs will query the ensxx net device from interface table, but nothing. So ovs will run the port_del operation.
- this will cause|OVS_VPORT_CMD_NEW| and OVS_VPORT_CMD_DEL operation to run repeatedly.
So the above issue can be avoided by the patch.
在 4/15/24 18:04, Eelco Chaudron 写道:
>
> On 13 Apr 2024, at 10:48, jun.gu wrote:
>
>> Check vport name from dev_get_by_name, this can avoid to add and remove
>> NIC repeatedly when NIC rename failed at system startup.
>>
>> Signed-off-by: Jun Gu<jun.gu@easystack.cn>
>> ---
>> net/openvswitch/vport-netdev.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
>> index 903537a5da22..de8977d7f329 100644
>> --- a/net/openvswitch/vport-netdev.c
>> +++ b/net/openvswitch/vport-netdev.c
>> @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name)
>> int err;
>>
>> vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name);
>> - if (!vport->dev) {
>> + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) {
> Hi Jun, not sure if I get the point here, as ovs_vport_name() translates into vport->dev->name.
>
> So are we trying to catch the interface rename between the dev_get_by_name(), and the code below? This rename could happen at any other place, so this check does not guarantee anything. Or am I missing something?
>
>> err = -ENODEV;
>> goto error_free_vport;
>> }
>> --
>> 2.25.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
On 16 Apr 2024, at 9:57, Jun Gu wrote: > Hi Chaudron, thanks for your reply. Considerthe following scenario: > > - set a net device alias name (such as *enpxx*) into ovs. > > - |OVS_VPORT_CMD_NEW -> ||dev_get_by_name can query the net device using *enpxx* name, but the dev->name that return is *ensxx* name. the |net_device|struct including: ``` |struct net_device { > char name[IFNAMSIZ]; > RH_KABI_REPLACE(struct hlist_node name_hlist, > struct netdev_name_node *name_node) > ... > |``` name_hlist include enpxx and ensxx name and both of them point to the same net_device, the net_device's name is ensxx. So when using *enpxx* name to query net device, the ||net device's name that return is *ensxx* name.| > Then, openvswitch.ko will save the net device that name is*ensxx* to a hash table. So this will cause the below process: > - ovs can'tobtain the enpxx net device by |OVS_VPORT_CMD_GET.| > -|dpif_netlink_port_poll will get a |notification after |OVS_VPORT_CMD_NEW|, but the vport's name is ensxx. ovs will query the ensxx net device from interface table, but nothing. So ovs will run the port_del operation. > - this will cause|OVS_VPORT_CMD_NEW| and OVS_VPORT_CMD_DEL operation to run repeatedly. > So the above issue can be avoided by the patch. Thanks for the clarification, I forgot about the alias case. And you are right, OVS userspace does not support using interface aliases. Maybe you can update the commit message, and add a code comment to clarify this in your next revision? Cheers, Eelco > 在 4/15/24 18:04, Eelco Chaudron 写道: >> >> On 13 Apr 2024, at 10:48, jun.gu wrote: >> >>> Check vport name from dev_get_by_name, this can avoid to add and remove >>> NIC repeatedly when NIC rename failed at system startup. >>> >>> Signed-off-by: Jun Gu<jun.gu@easystack.cn> >>> --- >>> net/openvswitch/vport-netdev.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c >>> index 903537a5da22..de8977d7f329 100644 >>> --- a/net/openvswitch/vport-netdev.c >>> +++ b/net/openvswitch/vport-netdev.c >>> @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name) >>> int err; >>> >>> vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name); >>> - if (!vport->dev) { >>> + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) { >> Hi Jun, not sure if I get the point here, as ovs_vport_name() translates into vport->dev->name. >> >> So are we trying to catch the interface rename between the dev_get_by_name(), and the code below? This rename could happen at any other place, so this check does not guarantee anything. Or am I missing something? >> >>> err = -ENODEV; >>> goto error_free_vport; >>> } >>> -- >>> 2.25.1 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On Sat, 2024-04-13 at 16:48 +0800, jun.gu wrote: > Check vport name from dev_get_by_name, this can avoid to add and remove > NIC repeatedly when NIC rename failed at system startup. > > Signed-off-by: Jun Gu <jun.gu@easystack.cn> > --- > net/openvswitch/vport-netdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c > index 903537a5da22..de8977d7f329 100644 > --- a/net/openvswitch/vport-netdev.c > +++ b/net/openvswitch/vport-netdev.c > @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name) > int err; > > vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name); > - if (!vport->dev) { > + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) { ^^ Missing open bracket here, and close bracket towards the end. Please build and test your patch before submitting the next revision. Please also include the target tree ('net-next') in the subj prefix when you will submit it. And update the commit message as per Eelco's request. Thanks, Paolo
diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c index 903537a5da22..de8977d7f329 100644 --- a/net/openvswitch/vport-netdev.c +++ b/net/openvswitch/vport-netdev.c @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name) int err; vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name); - if (!vport->dev) { + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) { err = -ENODEV; goto error_free_vport; }
Check vport name from dev_get_by_name, this can avoid to add and remove NIC repeatedly when NIC rename failed at system startup. Signed-off-by: Jun Gu <jun.gu@easystack.cn> --- net/openvswitch/vport-netdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)