From: Feng Zhou <[email protected]>
Add get_strings, get_sset_count, get_ethtool_stats to get peer
ifindex.
ethtool -S nk1
NIC statistics:
peer_ifindex: 36
Add get_link, get_link_ksettings to get link stat.
ethtool nk1
Settings for nk1:
...
Link detected: yes
Add get_ts_info.
ethtool -T nk1
Time stamping parameters for nk1:
...
Signed-off-by: Feng Zhou <[email protected]>
---
drivers/net/netkit.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c
index 97bd6705c241..1a5199568a07 100644
--- a/drivers/net/netkit.c
+++ b/drivers/net/netkit.c
@@ -34,6 +34,12 @@ struct netkit_link {
u32 location;
};
+static struct {
+ const char string[ETH_GSTRING_LEN];
+} ethtool_stats_keys[] = {
+ { "peer_ifindex" },
+};
+
static __always_inline int
netkit_run(const struct bpf_mprog_entry *entry, struct sk_buff *skb,
enum netkit_action ret)
@@ -211,8 +217,55 @@ static void netkit_get_drvinfo(struct net_device *dev,
strscpy(info->driver, DRV_NAME, sizeof(info->driver));
}
+static void netkit_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
+{
+ u8 *p = buf;
+
+ switch (stringset) {
+ case ETH_SS_STATS:
+ memcpy(p, ðtool_stats_keys, sizeof(ethtool_stats_keys));
+ p += sizeof(ethtool_stats_keys);
+ break;
+ }
+}
+
+static int netkit_get_sset_count(struct net_device *dev, int sset)
+{
+ switch (sset) {
+ case ETH_SS_STATS:
+ return ARRAY_SIZE(ethtool_stats_keys);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static void netkit_get_ethtool_stats(struct net_device *dev,
+ struct ethtool_stats *stats, u64 *data)
+{
+ struct netkit *nk = netkit_priv(dev);
+ struct net_device *peer = rtnl_dereference(nk->peer);
+
+ data[0] = peer ? peer->ifindex : 0;
+}
+
+static int netkit_get_link_ksettings(struct net_device *dev,
+ struct ethtool_link_ksettings *cmd)
+{
+ cmd->base.speed = SPEED_10000;
+ cmd->base.duplex = DUPLEX_FULL;
+ cmd->base.port = PORT_TP;
+ cmd->base.autoneg = AUTONEG_DISABLE;
+ return 0;
+}
+
static const struct ethtool_ops netkit_ethtool_ops = {
.get_drvinfo = netkit_get_drvinfo,
+ .get_link = ethtool_op_get_link,
+ .get_strings = netkit_get_strings,
+ .get_sset_count = netkit_get_sset_count,
+ .get_ethtool_stats = netkit_get_ethtool_stats,
+ .get_link_ksettings = netkit_get_link_ksettings,
+ .get_ts_info = ethtool_op_get_ts_info,
};
static void netkit_setup(struct net_device *dev)
--
2.30.2
On 11/30/23 09:58, Feng zhou wrote:
> From: Feng Zhou <[email protected]>
>
> Add get_strings, get_sset_count, get_ethtool_stats to get peer
> ifindex.
> ethtool -S nk1
> NIC statistics:
> peer_ifindex: 36
>
> Add get_link, get_link_ksettings to get link stat.
> ethtool nk1
> Settings for nk1:
> ...
> Link detected: yes
>
> Add get_ts_info.
> ethtool -T nk1
> Time stamping parameters for nk1:
> ...
>
> Signed-off-by: Feng Zhou <[email protected]>
> ---
> drivers/net/netkit.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
Hi,
I don't see any point in sending peer_ifindex through ethtool, even
worse through ethtool stats. That is definitely the wrong place for it.
You can already retrieve that through netlink. About the speed/duplex
this one makes more sense, but this is the wrong way to do it.
See how we did it for virtio_net (you are free to set speed/duplex
to anything to please bonding for example). Although I doubt anyone will
use netkit with bonding, so even that is questionable. :)
Nacked-by: Nikolay Aleksandrov <[email protected]>
Cheers,
Nik
在 2023/11/30 17:06, Nikolay Aleksandrov 写道:
> On 11/30/23 09:58, Feng zhou wrote:
>> From: Feng Zhou <[email protected]>
>>
>> Add get_strings, get_sset_count, get_ethtool_stats to get peer
>> ifindex.
>> ethtool -S nk1
>> NIC statistics:
>> peer_ifindex: 36
>>
>> Add get_link, get_link_ksettings to get link stat.
>> ethtool nk1
>> Settings for nk1:
>> ...
>> Link detected: yes
>>
>> Add get_ts_info.
>> ethtool -T nk1
>> Time stamping parameters for nk1:
>> ...
>>
>> Signed-off-by: Feng Zhou <[email protected]>
>> ---
>> drivers/net/netkit.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 53 insertions(+)
>>
>
> Hi,
> I don't see any point in sending peer_ifindex through ethtool, even
> worse through ethtool stats. That is definitely the wrong place for it.
> You can already retrieve that through netlink. About the speed/duplex
> this one makes more sense, but this is the wrong way to do it.
> See how we did it for virtio_net (you are free to set speed/duplex
> to anything to please bonding for example). Although I doubt anyone will
> use netkit with bonding, so even that is questionable. :)
>
> Nacked-by: Nikolay Aleksandrov <[email protected]>
>
> Cheers,
> Nik
>
We use netkit to replace veth to improve performance, veth can be used
ethtool -S veth to get peer_ifindex, so this part is added, as long as
it is to keep the netkit part and veth unified, to ensure the same usage
habits, and to replace it without perception.
On 11/30/23 10:24 AM, Feng Zhou wrote:
> 在 2023/11/30 17:06, Nikolay Aleksandrov 写道:
>> On 11/30/23 09:58, Feng zhou wrote:
>>> From: Feng Zhou <[email protected]>
>>>
>>> Add get_strings, get_sset_count, get_ethtool_stats to get peer
>>> ifindex.
>>> ethtool -S nk1
>>> NIC statistics:
>>> peer_ifindex: 36
>>>
>>> Add get_link, get_link_ksettings to get link stat.
>>> ethtool nk1
>>> Settings for nk1:
>>> ...
>>> Link detected: yes
>>>
>>> Add get_ts_info.
>>> ethtool -T nk1
>>> Time stamping parameters for nk1:
>>> ...
>>>
>>> Signed-off-by: Feng Zhou <[email protected]>
>>> ---
>>> drivers/net/netkit.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 53 insertions(+)
>>>
>>
>> Hi,
>> I don't see any point in sending peer_ifindex through ethtool, even
>> worse through ethtool stats. That is definitely the wrong place for it.
>> You can already retrieve that through netlink. About the speed/duplex
>> this one makes more sense, but this is the wrong way to do it.
>> See how we did it for virtio_net (you are free to set speed/duplex
>> to anything to please bonding for example). Although I doubt anyone will use netkit with bonding, so even that is questionable. :)
>>
>> Nacked-by: Nikolay Aleksandrov <[email protected]>
>
> We use netkit to replace veth to improve performance, veth can be used ethtool -S veth to get peer_ifindex, so this part is added, as long as it is to keep the netkit part and veth unified, to ensure the same usage habits, and to replace it without perception.
Could you elaborate some more on the use case why you need to retrieve it
via ethtool, what alternatives were tried and don't work?
Please also elaborate on the case for netkit_get_link_ksettings() and which
concrete problem you are trying to address with this extension?
The commit message only explains what is done but does not go into the detail
of _why_ you need it.
Thanks,
Daniel
在 2023/11/30 18:56, Daniel Borkmann 写道:
> On 11/30/23 10:24 AM, Feng Zhou wrote:
>> 在 2023/11/30 17:06, Nikolay Aleksandrov 写道:
>>> On 11/30/23 09:58, Feng zhou wrote:
>>>> From: Feng Zhou <[email protected]>
>>>>
>>>> Add get_strings, get_sset_count, get_ethtool_stats to get peer
>>>> ifindex.
>>>> ethtool -S nk1
>>>> NIC statistics:
>>>> peer_ifindex: 36
>>>>
>>>> Add get_link, get_link_ksettings to get link stat.
>>>> ethtool nk1
>>>> Settings for nk1:
>>>> ...
>>>> Link detected: yes
>>>>
>>>> Add get_ts_info.
>>>> ethtool -T nk1
>>>> Time stamping parameters for nk1:
>>>> ...
>>>>
>>>> Signed-off-by: Feng Zhou <[email protected]>
>>>> ---
>>>> drivers/net/netkit.c | 53
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 53 insertions(+)
>>>>
>>>
>>> Hi,
>>> I don't see any point in sending peer_ifindex through ethtool, even
>>> worse through ethtool stats. That is definitely the wrong place for it.
>>> You can already retrieve that through netlink. About the speed/duplex
>>> this one makes more sense, but this is the wrong way to do it.
>>> See how we did it for virtio_net (you are free to set speed/duplex
>>> to anything to please bonding for example). Although I doubt anyone
>>> will use netkit with bonding, so even that is questionable. :)
>>>
>>> Nacked-by: Nikolay Aleksandrov <[email protected]>
>>
>> We use netkit to replace veth to improve performance, veth can be used
>> ethtool -S veth to get peer_ifindex, so this part is added, as long as
>> it is to keep the netkit part and veth unified, to ensure the same
>> usage habits, and to replace it without perception.
>
> Could you elaborate some more on the use case why you need to retrieve it
> via ethtool, what alternatives were tried and don't work?
>
> Please also elaborate on the case for netkit_get_link_ksettings() and which
> concrete problem you are trying to address with this extension?
>
> The commit message only explains what is done but does not go into the
> detail
> of _why_ you need it.
>
> Thanks,
> Daniel
In general, this information can be obtained through ip commands or
netlink, and netkit_get_link_ksettings really not necessary. The reason
why ethtool supports this is that when we use veth, our business
colleagues are used to using ethtool to obtain peer_ifindex, and then
replace netkit, found that it could not be used, resulting in their
script failure, so they asked us for a request.
On 12/1/23 9:42 AM, Feng Zhou wrote:
> 在 2023/11/30 18:56, Daniel Borkmann 写道:
>> On 11/30/23 10:24 AM, Feng Zhou wrote:
>>> 在 2023/11/30 17:06, Nikolay Aleksandrov 写道:
>>>> On 11/30/23 09:58, Feng zhou wrote:
>>>>> From: Feng Zhou <[email protected]>
>>>>>
>>>>> Add get_strings, get_sset_count, get_ethtool_stats to get peer
>>>>> ifindex.
>>>>> ethtool -S nk1
>>>>> NIC statistics:
>>>>> peer_ifindex: 36
>>>>>
>>>>> Add get_link, get_link_ksettings to get link stat.
>>>>> ethtool nk1
>>>>> Settings for nk1:
>>>>> ...
>>>>> Link detected: yes
>>>>>
>>>>> Add get_ts_info.
>>>>> ethtool -T nk1
>>>>> Time stamping parameters for nk1:
>>>>> ...
>>>>>
>>>>> Signed-off-by: Feng Zhou <[email protected]>
>>>>> ---
>>>>> drivers/net/netkit.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 53 insertions(+)
>>>>
>>>> I don't see any point in sending peer_ifindex through ethtool, even
>>>> worse through ethtool stats. That is definitely the wrong place for it.
>>>> You can already retrieve that through netlink. About the speed/duplex
>>>> this one makes more sense, but this is the wrong way to do it.
>>>> See how we did it for virtio_net (you are free to set speed/duplex
>>>> to anything to please bonding for example). Although I doubt anyone will use netkit with bonding, so even that is questionable. :)
>>>>
>>>> Nacked-by: Nikolay Aleksandrov <[email protected]>
>>>
>>> We use netkit to replace veth to improve performance, veth can be used ethtool -S veth to get peer_ifindex, so this part is added, as long as it is to keep the netkit part and veth unified, to ensure the same usage habits, and to replace it without perception.
>>
>> Could you elaborate some more on the use case why you need to retrieve it
>> via ethtool, what alternatives were tried and don't work?
>>
>> Please also elaborate on the case for netkit_get_link_ksettings() and which
>> concrete problem you are trying to address with this extension?
>>
>> The commit message only explains what is done but does not go into the detail
>> of _why_ you need it.
>
> In general, this information can be obtained through ip commands or netlink, and netkit_get_link_ksettings really not necessary. The reason why ethtool supports this is that when we use veth, our business colleagues are used to using ethtool to obtain peer_ifindex, and then replace netkit, found that it could not be used, resulting in their script failure, so they asked us for a request.
Thanks, so the netkit_get_link_ksettings is optional. I don't quite follow what you
mean with regards to your business logic in veth to obtain peer ifindex. What does
the script do exactly with the peer ifindex (aka /why/ is it needed), could you
elaborate some more - it's still somewhat too vague? :) E.g. why it does not suffice
to look at the device type or other kind of attributes?
Thanks,
Daniel
在 2023/12/4 23:22, Daniel Borkmann 写道:
> Thanks, so the netkit_get_link_ksettings is optional.
Yes, netkit_get_link_ksettings really not necessary, I just added it in
line with veth.
I don't quite
> follow what you
> mean with regards to your business logic in veth to obtain peer ifindex.
> What does
> the script do exactly with the peer ifindex (aka /why/ is it needed),
> could you
> elaborate some more - it's still somewhat too vague? ???? E.g. why it does
> not suffice
> to look at the device type or other kind of attributes?
The scripting logic of the business colleagues should just be simple
logging records, using ethtool. Then they saw that netkit has this
missing, so raised this requirement, so I sent this patch, wanting to
hear your opinions. If you don't think it's necessary, let the business
colleagues modify it.
Thanks.
On 12/6/23 7:59 AM, Feng Zhou wrote:
> 在 2023/12/4 23:22, Daniel Borkmann 写道:
>> Thanks, so the netkit_get_link_ksettings is optional.
>
> Yes, netkit_get_link_ksettings really not necessary, I just added it in line with veth.
>
> I don't quite
>> follow what you
>> mean with regards to your business logic in veth to obtain peer ifindex. What does
>> the script do exactly with the peer ifindex (aka /why/ is it needed), could you
>> elaborate some more - it's still somewhat too vague? ???? E.g. why it does not suffice
>> to look at the device type or other kind of attributes?
>
> The scripting logic of the business colleagues should just be simple logging records, using ethtool. Then they saw that netkit has this missing, so raised this requirement, so I sent this patch, wanting to hear your opinions. If you don't think it's necessary, let the business colleagues modify it.
So it is basically only logging the peer_ifindex data from ethtool but nothing
more is done with it? Meaning, this was raised as a requirement because this was
missing from the logging data, or was there anything more to it?
The peer ifindex is already available via IFLA_LINK (which does dev_get_iflink()
internally to fetch the peer's ifindex). This is also what you see in iproute2:
# ip l
[...]
163: nk0@nk1: <BROADCAST,MULTICAST,NOARP,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
164: nk1@nk0: <BROADCAST,MULTICAST,NOARP,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
[...]
# ip netns add blue
# ip link set nk1 netns blue
# ip l
[...]
163: nk0@if164: <BROADCAST,MULTICAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000
link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff link-netns blue
[...]
The `if164` denotes the peer's ifindex and `link-netns blue` provides info on the netns of the peer.
This is much better and more generic than the ethtool hack. I would suggest changing the logic to
rather look at data populated by rtnl_fill_link_netnsid() instead.
Thanks,
Daniel
在 2023/12/6 18:57, Daniel Borkmann 写道:
> On 12/6/23 7:59 AM, Feng Zhou wrote:
>> 在 2023/12/4 23:22, Daniel Borkmann 写道:
>>> Thanks, so the netkit_get_link_ksettings is optional.
>>
>> Yes, netkit_get_link_ksettings really not necessary, I just added it
>> in line with veth.
>>
>> I don't quite
>>> follow what you
>>> mean with regards to your business logic in veth to obtain peer
>>> ifindex. What does
>>> the script do exactly with the peer ifindex (aka /why/ is it needed),
>>> could you
>>> elaborate some more - it's still somewhat too vague? ???? E.g. why it
>>> does not suffice
>>> to look at the device type or other kind of attributes?
>>
>> The scripting logic of the business colleagues should just be simple
>> logging records, using ethtool. Then they saw that netkit has this
>> missing, so raised this requirement, so I sent this patch, wanting to
>> hear your opinions. If you don't think it's necessary, let the
>> business colleagues modify it.
>
> So it is basically only logging the peer_ifindex data from ethtool but
> nothing
> more is done with it? Meaning, this was raised as a requirement because
> this was
> missing from the logging data, or was there anything more to it?
>
> The peer ifindex is already available via IFLA_LINK (which does
> dev_get_iflink()
> internally to fetch the peer's ifindex). This is also what you see in
> iproute2:
>
> # ip l
> [...]
> 163: nk0@nk1: <BROADCAST,MULTICAST,NOARP,M-DOWN> mtu 1500 qdisc noop
> state DOWN group default qlen 1000
> link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
> 164: nk1@nk0: <BROADCAST,MULTICAST,NOARP,M-DOWN> mtu 1500 qdisc noop
> state DOWN group default qlen 1000
> link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
> [...]
> # ip netns add blue
> # ip link set nk1 netns blue
> # ip l
> [...]
> 163: nk0@if164: <BROADCAST,MULTICAST,NOARP> mtu 1500 qdisc noop state
> DOWN group default qlen 1000
> link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff link-netns blue
> [...]
>
> The `if164` denotes the peer's ifindex and `link-netns blue` provides
> info on the netns of the peer.
> This is much better and more generic than the ethtool hack. I would
> suggest changing the logic to
> rather look at data populated by rtnl_fill_link_netnsid() instead.
>
> Thanks,
> Daniel
Yes, if veth supports ethtool -S to get the function of peer's ifindex,
it is not necessary for netkit and can be replaced by other ways.