[PATCHv2 net-next] bonding: add software tx timestamping support

[PATCHv2 net-next] bonding: add software tx timestamping support

[Hangbin Liu]

Currently, bonding only obtain the timestamp (ts) information of
the active slave, which is available only for modes 1, 5, and 6.
For other modes, bonding only has software rx timestamping support.

However, some users who use modes such as LACP also want tx timestamp
support. To address this issue, let's check the ts information of each
slave. If all slaves support tx timestamping, we can enable tx
timestamping support for the bond.

Suggested-by: Miroslav Lichvar <mlichvar@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 39 +++++++++++++++++++++++++++++++--
 include/uapi/linux/net_tstamp.h |  3 +++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 00646aa315c3..994efc777a77 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5686,9 +5686,13 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 				    struct ethtool_ts_info *info)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
+	struct ethtool_ts_info ts_info;
 	const struct ethtool_ops *ops;
 	struct net_device *real_dev;
 	struct phy_device *phydev;
+	bool soft_support = false;
+	struct list_head *iter;
+	struct slave *slave;
 	int ret = 0;
 
 	rcu_read_lock();
@@ -5707,10 +5711,41 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 			ret = ops->get_ts_info(real_dev, info);
 			goto out;
 		}
+	} else {
+		/* Check if all slaves support software rx/tx timestamping */
+		rcu_read_lock();
+		bond_for_each_slave_rcu(bond, slave, iter) {
+			ret = -1;
+			dev_hold(slave->dev);
+			ops = slave->dev->ethtool_ops;
+			phydev = slave->dev->phydev;
+
+			if (phy_has_tsinfo(phydev))
+				ret = phy_ts_info(phydev, &ts_info);
+			else if (ops->get_ts_info)
+				ret = ops->get_ts_info(slave->dev, &ts_info);
+
+			if (!ret && (ts_info.so_timestamping & SOF_TIMESTAMPING_SOFTRXTX) == \
+				    SOF_TIMESTAMPING_SOFTRXTX) {
+				dev_put(slave->dev);
+				soft_support = true;
+				continue;
+			}
+
+			soft_support = false;
+			dev_put(slave->dev);
+			break;
+		}
+		rcu_read_unlock();
 	}
 
-	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
-				SOF_TIMESTAMPING_SOFTWARE;
+	ret = 0;
+	if (soft_support) {
+		info->so_timestamping = SOF_TIMESTAMPING_SOFTRXTX;
+	} else {
+		info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
+					SOF_TIMESTAMPING_SOFTWARE;
+	}
 	info->phc_index = -1;
 
 out:
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index a2c66b3d7f0f..2adaa0008434 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -48,6 +48,9 @@ enum {
 					 SOF_TIMESTAMPING_TX_SCHED | \
 					 SOF_TIMESTAMPING_TX_ACK)
 
+#define SOF_TIMESTAMPING_SOFTRXTX (SOF_TIMESTAMPING_TX_SOFTWARE | \
+				   SOF_TIMESTAMPING_RX_SOFTWARE | \
+				   SOF_TIMESTAMPING_SOFTWARE)
 /**
  * struct so_timestamping - SO_TIMESTAMPING parameter
  *
-- 
2.38.1

Re: [PATCHv2 net-next] bonding: add software tx timestamping support

[Eric Dumazet]

On Fri, Apr 7, 2023 at 8:12 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Currently, bonding only obtain the timestamp (ts) information of
> the active slave, which is available only for modes 1, 5, and 6.
> For other modes, bonding only has software rx timestamping support.
>
> However, some users who use modes such as LACP also want tx timestamp
> support. To address this issue, let's check the ts information of each
> slave. If all slaves support tx timestamping, we can enable tx
> timestamping support for the bond.
>
> Suggested-by: Miroslav Lichvar <mlichvar@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 39 +++++++++++++++++++++++++++++++--
>  include/uapi/linux/net_tstamp.h |  3 +++
>  2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 00646aa315c3..994efc777a77 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5686,9 +5686,13 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
>                                     struct ethtool_ts_info *info)
>  {
>         struct bonding *bond = netdev_priv(bond_dev);
> +       struct ethtool_ts_info ts_info;
>         const struct ethtool_ops *ops;
>         struct net_device *real_dev;
>         struct phy_device *phydev;
> +       bool soft_support = false;
> +       struct list_head *iter;
> +       struct slave *slave;
>         int ret = 0;
>
>         rcu_read_lock();
> @@ -5707,10 +5711,41 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
>                         ret = ops->get_ts_info(real_dev, info);
>                         goto out;
>                 }
> +       } else {
> +               /* Check if all slaves support software rx/tx timestamping */
> +               rcu_read_lock();
> +               bond_for_each_slave_rcu(bond, slave, iter) {
> +                       ret = -1;
> +                       dev_hold(slave->dev);

You are holding rcu_read_lock() during the loop, so there is no need
for this dev_hold() / dev_put() dance.

And if this was needed, we kindly ask for new dev_hold()/dev_put()
added in networking code to
instead use netdev_hold / netdev_put(), we have spent enough time
tracking hold/put bugs.

Thanks.


> +                       ops = slave->dev->ethtool_ops;
> +                       phydev = slave->dev->phydev;
> +
> +                       if (phy_has_tsinfo(phydev))
> +                               ret = phy_ts_info(phydev, &ts_info);
> +                       else if (ops->get_ts_info)
> +                               ret = ops->get_ts_info(slave->dev, &ts_info);
> +
> +                       if (!ret && (ts_info.so_timestamping & SOF_TIMESTAMPING_SOFTRXTX) == \
> +                                   SOF_TIMESTAMPING_SOFTRXTX) {
> +                               dev_put(slave->dev);
> +                               soft_support = true;
> +                               continue;
> +                       }
> +
> +                       soft_support = false;
> +                       dev_put(slave->dev);
> +                       break;
> +               }
> +               rcu_read_unlock();

Re: [PATCHv2 net-next] bonding: add software tx timestamping support

[Simon Horman]

On Fri, Apr 07, 2023 at 02:12:28PM +0800, Hangbin Liu wrote:
> Currently, bonding only obtain the timestamp (ts) information of
> the active slave, which is available only for modes 1, 5, and 6.
> For other modes, bonding only has software rx timestamping support.
> 
> However, some users who use modes such as LACP also want tx timestamp
> support. To address this issue, let's check the ts information of each
> slave. If all slaves support tx timestamping, we can enable tx
> timestamping support for the bond.
> 
> Suggested-by: Miroslav Lichvar <mlichvar@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

...

> @@ -5707,10 +5711,41 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
>  			ret = ops->get_ts_info(real_dev, info);
>  			goto out;
>  		}
> +	} else {
> +		/* Check if all slaves support software rx/tx timestamping */
> +		rcu_read_lock();
> +		bond_for_each_slave_rcu(bond, slave, iter) {
> +			ret = -1;
> +			dev_hold(slave->dev);
> +			ops = slave->dev->ethtool_ops;
> +			phydev = slave->dev->phydev;
> +
> +			if (phy_has_tsinfo(phydev))
> +				ret = phy_ts_info(phydev, &ts_info);
> +			else if (ops->get_ts_info)
> +				ret = ops->get_ts_info(slave->dev, &ts_info);
> +
> +			if (!ret && (ts_info.so_timestamping & SOF_TIMESTAMPING_SOFTRXTX) == \

nit: no need for the '\' for line continuation.

> +				    SOF_TIMESTAMPING_SOFTRXTX) {
> +				dev_put(slave->dev);
> +				soft_support = true;
> +				continue;
> +			}
> +
> +			soft_support = false;
> +			dev_put(slave->dev);
> +			break;
> +		}
> +		rcu_read_unlock();
>  	}

Re: [PATCHv2 net-next] bonding: add software tx timestamping support

[Hangbin Liu]

On Fri, Apr 07, 2023 at 11:34:23AM +0200, Eric Dumazet wrote:
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 00646aa315c3..994efc777a77 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -5686,9 +5686,13 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
> >                                     struct ethtool_ts_info *info)
> >  {
> >         struct bonding *bond = netdev_priv(bond_dev);
> > +       struct ethtool_ts_info ts_info;
> >         const struct ethtool_ops *ops;
> >         struct net_device *real_dev;
> >         struct phy_device *phydev;
> > +       bool soft_support = false;
> > +       struct list_head *iter;
> > +       struct slave *slave;
> >         int ret = 0;
> >
> >         rcu_read_lock();
> > @@ -5707,10 +5711,41 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
> >                         ret = ops->get_ts_info(real_dev, info);
> >                         goto out;
> >                 }
> > +       } else {
> > +               /* Check if all slaves support software rx/tx timestamping */
> > +               rcu_read_lock();
> > +               bond_for_each_slave_rcu(bond, slave, iter) {
> > +                       ret = -1;
> > +                       dev_hold(slave->dev);
> 
> You are holding rcu_read_lock() during the loop, so there is no need
> for this dev_hold() / dev_put() dance.

Thanks, I will remove this part.

> 
> And if this was needed, we kindly ask for new dev_hold()/dev_put()
> added in networking code to
> instead use netdev_hold / netdev_put(), we have spent enough time
> tracking hold/put bugs.

I saw dev_hold has called netdev_hold. Is there a need to convert the
old dev_hold to netdev_hold in driver code?

Thanks
Hangbin