| Message ID | 20240408180918.2773238-1-jfraker@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] gve: Correctly report software timestamping capabilities | expand |
John Fraker wrote: > gve has supported software timestamp generation since its inception, > but has not advertised that support via ethtool. This patch correctly > advertises that support. > > Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com> > Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com> > Signed-off-by: John Fraker <jfraker@google.com> Reviewed-by: Willem de Bruijn <willemb@google.com> > > --- > drivers/net/ethernet/google/gve/gve_ethtool.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c > index 815dead..99f5aeb 100644 > --- a/drivers/net/ethernet/google/gve/gve_ethtool.c > +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c > @@ -4,6 +4,8 @@ > * Copyright (C) 2015-2021 Google, Inc. > */ > > +#include <linux/ethtool.h> > +#include <linux/net_tstamp.h> > #include <linux/rtnetlink.h> > #include "gve.h" > #include "gve_adminq.h" > @@ -763,6 +765,15 @@ static int gve_set_coalesce(struct net_device *netdev, > return 0; > } > > +static int gve_get_ts_info(struct net_device *netdev, struct ethtool_ts_info *info) > +{ > + info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | > + SOF_TIMESTAMPING_TX_SOFTWARE | > + SOF_TIMESTAMPING_SOFTWARE; > + > + return 0; This device calls skb_tx_timestamp in its ndo_start_xmit: the prerequisite for SOF_TIMESTAMPING_TX_SOFTWARE. All devices support SOF_TIMESTAMPING_RX_SOFTWARE by virtue of net_timestamp_check being called in the device independent code. To ethtool timestamping maintainers: It's quite unnecessary to have each device advertise SOF_TIMESTAMPING_RX_SOFTWARE | SOF_TIMESTAMPING_SOFTWARE. In __ethtool_get_ts_info we could just always add those flags to the result from the callees. if (phy_has_tsinfo(phydev)) return phy_ts_info(phydev, info); if (ops->get_ts_info) return ops->get_ts_info(dev, info); info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | SOF_TIMESTAMPING_SOFTWARE; > +} > + > const struct ethtool_ops gve_ethtool_ops = { > .supported_coalesce_params = ETHTOOL_COALESCE_USECS, > .supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT, > @@ -784,5 +795,6 @@ const struct ethtool_ops gve_ethtool_ops = { > .set_tunable = gve_set_tunable, > .get_priv_flags = gve_get_priv_flags, > .set_priv_flags = gve_set_priv_flags, > - .get_link_ksettings = gve_get_link_ksettings > + .get_link_ksettings = gve_get_link_ksettings, > + .get_ts_info = gve_get_ts_info > }; > -- > 2.44.0.478.gd926399ef9-goog >
On Mon, 8 Apr 2024 11:09:01 -0700 John Fraker wrote: > gve has supported software timestamp generation since its inception, > but has not advertised that support via ethtool. This patch correctly > advertises that support. > > Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com> > Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com> > Signed-off-by: John Fraker <jfraker@google.com> I think it should be a single line diff: + .get_ts_info = ethtool_op_get_ts_info, right?
On Tue, 09 Apr 2024 10:29:55 -0400 Willem de Bruijn wrote: > This device calls skb_tx_timestamp in its ndo_start_xmit: the > prerequisite for SOF_TIMESTAMPING_TX_SOFTWARE. > > All devices support SOF_TIMESTAMPING_RX_SOFTWARE by virtue of > net_timestamp_check being called in the device independent code. > > To ethtool timestamping maintainers: It's quite unnecessary to have > each device advertise SOF_TIMESTAMPING_RX_SOFTWARE | > SOF_TIMESTAMPING_SOFTWARE. In __ethtool_get_ts_info we could just > always add those flags to the result from the callees. > > if (phy_has_tsinfo(phydev)) > return phy_ts_info(phydev, info); > if (ops->get_ts_info) > return ops->get_ts_info(dev, info); > > info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | > SOF_TIMESTAMPING_SOFTWARE; My gut tells me we force drivers to set the ethtool op because while at it they will probably also implement tx stamping. Even more unhelpful point I'll risk making is that we could add a test and make people who submit new drivers run it :)
On Tue, 09 Apr, 2024 17:28:38 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 09 Apr 2024 10:29:55 -0400 Willem de Bruijn wrote: >> This device calls skb_tx_timestamp in its ndo_start_xmit: the >> prerequisite for SOF_TIMESTAMPING_TX_SOFTWARE. >> >> All devices support SOF_TIMESTAMPING_RX_SOFTWARE by virtue of >> net_timestamp_check being called in the device independent code. >> >> To ethtool timestamping maintainers: It's quite unnecessary to have >> each device advertise SOF_TIMESTAMPING_RX_SOFTWARE | >> SOF_TIMESTAMPING_SOFTWARE. In __ethtool_get_ts_info we could just >> always add those flags to the result from the callees. >> >> if (phy_has_tsinfo(phydev)) >> return phy_ts_info(phydev, info); >> if (ops->get_ts_info) >> return ops->get_ts_info(dev, info); >> >> info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | >> SOF_TIMESTAMPING_SOFTWARE; > > My gut tells me we force drivers to set the ethtool op because > while at it they will probably also implement tx stamping. I think the logic should be the other way (in terms of the relationship). A call to skb_tx_timestamp should throw a warning if the driver does not advertise its timestamping capabilities. This way, a naive netdev driver for some lightweight device does not need to worry about this. I agree that anyone implementing tx timestamping should have this operation defined. An skb does not contain any mechanism to reference the driver's ethtool callback. Maybe the right choice is to have a ts capability function registered for each netdev that can be used by the core stack and that powers the ethtool operation as well instead of the existing callback for ethtool? > > Even more unhelpful point I'll risk making is that we could > add a test and make people who submit new drivers run it :) -- Thanks, Rahul Rameshbabu
On Tue, 09 Apr 2024 21:40:46 -0700 Rahul Rameshbabu wrote: > > My gut tells me we force drivers to set the ethtool op because > > while at it they will probably also implement tx stamping. > > I think the logic should be the other way (in terms of the > relationship). A call to skb_tx_timestamp should throw a warning if the > driver does not advertise its timestamping capabilities. This way, a > naive netdev driver for some lightweight device does not need to worry > about this. I agree that anyone implementing tx timestamping should have > this operation defined. An skb does not contain any mechanism to > reference the driver's ethtool callback. Maybe the right choice is to > have a ts capability function registered for each netdev that can be > used by the core stack and that powers the ethtool operation as well > instead of the existing callback for ethtool? Adding a check which only need to runs once in the lifetime of the driver to the fastpath may be a little awkward. Another option would be a sufficiently intelligent grep, which would understand which files constitute a driver. At which point grepping for the ethtool op and skb_tx_timestamp would be trivial?
Jakub Kicinski wrote: > On Tue, 09 Apr 2024 21:40:46 -0700 Rahul Rameshbabu wrote: > > > My gut tells me we force drivers to set the ethtool op because > > > while at it they will probably also implement tx stamping. > > > > I think the logic should be the other way (in terms of the > > relationship). A call to skb_tx_timestamp should throw a warning if the > > driver does not advertise its timestamping capabilities. This way, a > > naive netdev driver for some lightweight device does not need to worry > > about this. I agree that anyone implementing tx timestamping should have > > this operation defined. An skb does not contain any mechanism to > > reference the driver's ethtool callback. Maybe the right choice is to > > have a ts capability function registered for each netdev that can be > > used by the core stack and that powers the ethtool operation as well > > instead of the existing callback for ethtool? > > Adding a check which only need to runs once in the lifetime of > the driver to the fastpath may be a little awkward. Another option > would be a sufficiently intelligent grep, which would understand > which files constitute a driver. At which point grepping for > the ethtool op and skb_tx_timestamp would be trivial? Many may not define the flags themselves, but defer this to ethtool_op_get_ts_info. A not so much intelligent, but sufficiently ugly, grep indicates not a a massive amount of many missing entries among ethernet drivers. But this first attempt is definitely lossy. $ for symbol in skb_tx_timestamp get_ts_info SOF_TIMESTAMPING_TX_SOFTWARE ethtool_op_get_ts_info "(SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info)"; do echo -n "$symbol: "; for i in `grep -nrIE "$symbol" drivers/net/ethernet/ | awk '{print $1}' | xargs dirname | uniq`; do echo $i; done | wc -l; done skb_tx_timestamp: 69 get_ts_info: 66 SOF_TIMESTAMPING_TX_SOFTWARE: 33 ethtool_op_get_ts_info: 40 (SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info): 59 This does not add up, but that's because some drivers share prefixes, and some drivers have different paths where one open codes and the other calls ethtool_op_get_ts_info. Marvell is a good example of both: $ grep -nrIE '(SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info)' drivers/net/ethernet /marvell drivers/net/ethernet/marvell/pxa168_eth.c:1367: .get_ts_info = ethtool_op_get_ts_info, drivers/net/ethernet/marvell/mv643xx_eth.c:1756: .get_ts_info = ethtool_op_get_ts_info, drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:5266: info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE | drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:962: return ethtool_op_get_ts_info(netdev, info); drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:964: info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE | One more aside, no driver should have to advertise SOF_TIMESTAMPING_SOFTWARE or SOF_TIMESTAMPING_RAW_HARDWARE. Per Documentation/networking/timestamping.rst these are reporting flags, not recording flags. Devices only optionall record a timestamp.
On Wed, 10 Apr, 2024 15:31:56 -0400 Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > Jakub Kicinski wrote: >> On Tue, 09 Apr 2024 21:40:46 -0700 Rahul Rameshbabu wrote: >> > > My gut tells me we force drivers to set the ethtool op because >> > > while at it they will probably also implement tx stamping. >> > >> > I think the logic should be the other way (in terms of the >> > relationship). A call to skb_tx_timestamp should throw a warning if the >> > driver does not advertise its timestamping capabilities. This way, a >> > naive netdev driver for some lightweight device does not need to worry >> > about this. I agree that anyone implementing tx timestamping should have >> > this operation defined. An skb does not contain any mechanism to >> > reference the driver's ethtool callback. Maybe the right choice is to >> > have a ts capability function registered for each netdev that can be >> > used by the core stack and that powers the ethtool operation as well >> > instead of the existing callback for ethtool? >> >> Adding a check which only need to runs once in the lifetime of >> the driver to the fastpath may be a little awkward. Another option >> would be a sufficiently intelligent grep, which would understand >> which files constitute a driver. At which point grepping for >> the ethtool op and skb_tx_timestamp would be trivial? > > Many may not define the flags themselves, but defer this to > ethtool_op_get_ts_info. > > A not so much intelligent, but sufficiently ugly, grep indicates > not a a massive amount of many missing entries among ethernet > drivers. But this first attempt is definitely lossy. > > $ for symbol in skb_tx_timestamp get_ts_info SOF_TIMESTAMPING_TX_SOFTWARE ethtool_op_get_ts_info "(SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info)"; do > echo -n "$symbol: "; > for i in `grep -nrIE "$symbol" drivers/net/ethernet/ | awk '{print $1}' | xargs dirname | uniq`; do echo $i; done | wc -l; > done > > skb_tx_timestamp: 69 > get_ts_info: 66 > SOF_TIMESTAMPING_TX_SOFTWARE: 33 > ethtool_op_get_ts_info: 40 > (SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info): 59 > > This does not add up, but that's because some drivers share prefixes, > and some drivers have different paths where one open codes and the > other calls ethtool_op_get_ts_info. Marvell is a good example of both: > > $ grep -nrIE '(SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info)' drivers/net/ethernet > /marvell > drivers/net/ethernet/marvell/pxa168_eth.c:1367: .get_ts_info = ethtool_op_get_ts_info, > drivers/net/ethernet/marvell/mv643xx_eth.c:1756: .get_ts_info = ethtool_op_get_ts_info, > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:5266: info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE | > drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:962: return ethtool_op_get_ts_info(netdev, info); > drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:964: info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE | If there is a desire to enforce all drivers need to implement .get_is_info, would the following make sense? My biggest objection to this idea was mainly my concern that the drivers would miss setting info->so_timestamping with SOF_TIMESTAMPING_RX_SOFTWARE | SOF_TIMESTAMPING_SOFTWARE, which I do not think should be a responsibility of the driver author since this is happening in the core stack. So maybe something like this (taking Willem's proposal for __ethtool_get_ts_info and modifying it a bit)? int err = 0; ... info->phc_index = -1; if (phy_has_tsinfo(phydev)) err = phy_ts_info(phydev, info); else err = ops->get_ts_info(dev, info); info->so_timestamping |= SOF_TIMESTAMPING_RX_SOFTWARE | SOF_TIMESTAMPING_SOFTWARE; return err; > > One more aside, no driver should have to advertise > SOF_TIMESTAMPING_SOFTWARE or SOF_TIMESTAMPING_RAW_HARDWARE. Per > Documentation/networking/timestamping.rst these are reporting flags, > not recording flags. Devices only optionall record a timestamp. I think this view aligns with my opinion above (though good point about timestamping reporting bits in general should be deduced based on the timestamp generation bits set rather than needing to be set as well). -- Thanks, Rahul Rameshbabu
Rahul Rameshbabu wrote: > > On Wed, 10 Apr, 2024 15:31:56 -0400 Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Jakub Kicinski wrote: > >> On Tue, 09 Apr 2024 21:40:46 -0700 Rahul Rameshbabu wrote: > >> > > My gut tells me we force drivers to set the ethtool op because > >> > > while at it they will probably also implement tx stamping. > >> > > >> > I think the logic should be the other way (in terms of the > >> > relationship). A call to skb_tx_timestamp should throw a warning if the > >> > driver does not advertise its timestamping capabilities. This way, a > >> > naive netdev driver for some lightweight device does not need to worry > >> > about this. I agree that anyone implementing tx timestamping should have > >> > this operation defined. An skb does not contain any mechanism to > >> > reference the driver's ethtool callback. Maybe the right choice is to > >> > have a ts capability function registered for each netdev that can be > >> > used by the core stack and that powers the ethtool operation as well > >> > instead of the existing callback for ethtool? > >> > >> Adding a check which only need to runs once in the lifetime of > >> the driver to the fastpath may be a little awkward. Another option > >> would be a sufficiently intelligent grep, which would understand > >> which files constitute a driver. At which point grepping for > >> the ethtool op and skb_tx_timestamp would be trivial? > > > > Many may not define the flags themselves, but defer this to > > ethtool_op_get_ts_info. > > > > A not so much intelligent, but sufficiently ugly, grep indicates > > not a a massive amount of many missing entries among ethernet > > drivers. But this first attempt is definitely lossy. > > > > $ for symbol in skb_tx_timestamp get_ts_info SOF_TIMESTAMPING_TX_SOFTWARE ethtool_op_get_ts_info "(SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info)"; do > > echo -n "$symbol: "; > > for i in `grep -nrIE "$symbol" drivers/net/ethernet/ | awk '{print $1}' | xargs dirname | uniq`; do echo $i; done | wc -l; > > done > > > > skb_tx_timestamp: 69 > > get_ts_info: 66 > > SOF_TIMESTAMPING_TX_SOFTWARE: 33 > > ethtool_op_get_ts_info: 40 > > (SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info): 59 > > > > This does not add up, but that's because some drivers share prefixes, > > and some drivers have different paths where one open codes and the > > other calls ethtool_op_get_ts_info. Marvell is a good example of both: > > > > $ grep -nrIE '(SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info)' drivers/net/ethernet > > /marvell > > drivers/net/ethernet/marvell/pxa168_eth.c:1367: .get_ts_info = ethtool_op_get_ts_info, > > drivers/net/ethernet/marvell/mv643xx_eth.c:1756: .get_ts_info = ethtool_op_get_ts_info, > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:5266: info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE | > > drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:962: return ethtool_op_get_ts_info(netdev, info); > > drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:964: info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE | > > If there is a desire to enforce all drivers need to implement > .get_is_info, would the following make sense? The only reason to enforce this is if we want to enforce them to also implement tx software timestamping. Generally, these features are opt in. > My biggest objection to > this idea was mainly my concern that the drivers would miss setting > info->so_timestamping with SOF_TIMESTAMPING_RX_SOFTWARE | > SOF_TIMESTAMPING_SOFTWARE, which I do not think should be a > responsibility of the driver author since this is happening in the core > stack. > > So maybe something like this (taking Willem's proposal for > __ethtool_get_ts_info and modifying it a bit)? > > int err = 0; > > ... > > info->phc_index = -1; > > if (phy_has_tsinfo(phydev)) > err = phy_ts_info(phydev, info); > else > err = ops->get_ts_info(dev, info); > > info->so_timestamping |= SOF_TIMESTAMPING_RX_SOFTWARE | > SOF_TIMESTAMPING_SOFTWARE; > > return err; Yes, this is what I meant as well. (the code I showed was just copied verbatim from net-next as context, not a suggested change.) > > > > One more aside, no driver should have to advertise > > SOF_TIMESTAMPING_SOFTWARE or SOF_TIMESTAMPING_RAW_HARDWARE. Per > > Documentation/networking/timestamping.rst these are reporting flags, > > not recording flags. Devices only optionall record a timestamp. > > I think this view aligns with my opinion above (though good point about > timestamping reporting bits in general should be deduced based on the > timestamp generation bits set rather than needing to be set as well).
Jakub Kicinski wrote: > On Mon, 8 Apr 2024 11:09:01 -0700 John Fraker wrote: > > gve has supported software timestamp generation since its inception, > > but has not advertised that support via ethtool. This patch correctly > > advertises that support. > > > > Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com> > > Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com> > > Signed-off-by: John Fraker <jfraker@google.com> > > I think it should be a single line diff: > > + .get_ts_info = ethtool_op_get_ts_info, > > right? If inserted above .get_link_ksettings that works. The current ordering is not based on actual struct layout anyway. Probably all statements should just end in a comma, including a trailing comma. To avoid these two line changes on each subsequent change. The rest of the discussion in this thread is actually quite unrelated to this patch. Didn't meant to sidetrack that.
On Wed, Apr 10, 2024 at 8:22 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Jakub Kicinski wrote: > > On Mon, 8 Apr 2024 11:09:01 -0700 John Fraker wrote: > > > gve has supported software timestamp generation since its inception, > > > but has not advertised that support via ethtool. This patch correctly > > > advertises that support. > > > > > > Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com> > > > Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com> > > > Signed-off-by: John Fraker <jfraker@google.com> > > > > I think it should be a single line diff: > > > > + .get_ts_info = ethtool_op_get_ts_info, > > > > right? > > If inserted above .get_link_ksettings that works. The current > ordering is not based on actual struct layout anyway. > > Probably all statements should just end in a comma, including a > trailing comma. To avoid these two line changes on each subsequent > change. Thanks all! I'll send the one-line v2. > The rest of the discussion in this thread is actually quite > unrelated to this patch. Didn't meant to sidetrack that.
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c index 815dead..99f5aeb 100644 --- a/drivers/net/ethernet/google/gve/gve_ethtool.c +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c @@ -4,6 +4,8 @@ * Copyright (C) 2015-2021 Google, Inc. */ +#include <linux/ethtool.h> +#include <linux/net_tstamp.h> #include <linux/rtnetlink.h> #include "gve.h" #include "gve_adminq.h" @@ -763,6 +765,15 @@ static int gve_set_coalesce(struct net_device *netdev, return 0; } +static int gve_get_ts_info(struct net_device *netdev, struct ethtool_ts_info *info) +{ + info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | + SOF_TIMESTAMPING_TX_SOFTWARE | + SOF_TIMESTAMPING_SOFTWARE; + + return 0; +} + const struct ethtool_ops gve_ethtool_ops = { .supported_coalesce_params = ETHTOOL_COALESCE_USECS, .supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT, @@ -784,5 +795,6 @@ const struct ethtool_ops gve_ethtool_ops = { .set_tunable = gve_set_tunable, .get_priv_flags = gve_get_priv_flags, .set_priv_flags = gve_set_priv_flags, - .get_link_ksettings = gve_get_link_ksettings + .get_link_ksettings = gve_get_link_ksettings, + .get_ts_info = gve_get_ts_info };