| Message ID | 20241108094206.2808293-1-zhangjian.3032@bytedance.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] mctp i2c: notify user space on TX failure | expand |
On Fri, 8 Nov 2024 17:42:06 +0800 Jian Zhang wrote: > diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c > index 4dc057c121f5..e9a835606dfc 100644 > --- a/drivers/net/mctp/mctp-i2c.c > +++ b/drivers/net/mctp/mctp-i2c.c > @@ -485,6 +485,7 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb) > struct mctp_i2c_hdr *hdr; > struct i2c_msg msg = {0}; > u8 *pecp; > + struct sock *sk; > int rc; > nit: order the variable declaration lines longest to shortest > @@ -551,6 +552,14 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb) > dev_warn_ratelimited(&midev->adapter->dev, > "__i2c_transfer failed %d\n", rc); > stats->tx_errors++; > + > + sk = skb->sk; > + if (sk) { > + sk->sk_err = -rc; > + if (!sock_flag(sk, SOCK_DEAD)) > + sk_error_report(sk); > + } notifying socket in the xmit handler of a netdev is a bit strange, could you do it somewhere higher in the MCTP stack?
Hi Jakub, > > @@ -551,6 +552,14 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb) > > dev_warn_ratelimited(&midev->adapter->dev, > > "__i2c_transfer failed %d\n", rc); > > stats->tx_errors++; > > + > > + sk = skb->sk; > > + if (sk) { > > + sk->sk_err = -rc; > > + if (!sock_flag(sk, SOCK_DEAD)) > > + sk_error_report(sk); > > + } > > notifying socket in the xmit handler of a netdev is a bit strange, > could you do it somewhere higher in the MCTP stack? Sounds like that would be useful in general for MCTP, but we don't have a facility for that at present. Any existing implementation you would suggest modelling this on? Cheers, Jeremy
On Thu, 14 Nov 2024 11:13:33 +0800 Jeremy Kerr wrote: > > notifying socket in the xmit handler of a netdev is a bit strange, > > could you do it somewhere higher in the MCTP stack? > > Sounds like that would be useful in general for MCTP, but we don't have > a facility for that at present. Any existing implementation you would > suggest modelling this on? routing isn't really my forte, TBH, what eats the error so that it doesn't come out of mctp_local_output() ? Do you use qdiscs on top of the MCTP devices?
Hi Jakub, > routing isn't really my forte, TBH, what eats the error so that it > doesn't come out of mctp_local_output() ? Do you use qdiscs on top > of the MCTP devices? There are no qdiscs involved at this stage, as we need to preserve packet ordering in most cases. The route output functions will end up in a dev_queue_xmit, so any tx error would have been decoupled from the route output at that stage. I'm happy to do the exploring myself if you don't have strong opinions on an implementation. Cheers, Jeremy
On Thu, 14 Nov 2024 14:48:57 +0800 Jeremy Kerr wrote: > > routing isn't really my forte, TBH, what eats the error so that it > > doesn't come out of mctp_local_output() ? Do you use qdiscs on top > > of the MCTP devices? > > There are no qdiscs involved at this stage, as we need to preserve > packet ordering in most cases. The route output functions will end up > in a dev_queue_xmit, so any tx error would have been decoupled from the > route output at that stage. Ah, it's the driver eating the errors, it puts the packet on a local queue and returns OK no matter what. The I2C transfer happens from a thread. I wonder if there is precedent, let's ask CAN experts. Mark, MCTP would like to report errors from the drivers all the way to the socket. Do CAN drivers do something along these lines?
On 14.11.2024 07:02:35, Jakub Kicinski wrote: > On Thu, 14 Nov 2024 14:48:57 +0800 Jeremy Kerr wrote: > > > routing isn't really my forte, TBH, what eats the error so that it > > > doesn't come out of mctp_local_output() ? Do you use qdiscs on top > > > of the MCTP devices? > > > > There are no qdiscs involved at this stage, as we need to preserve > > packet ordering in most cases. The route output functions will end up > > in a dev_queue_xmit, so any tx error would have been decoupled from the > > route output at that stage. > > Ah, it's the driver eating the errors, it puts the packet on a local > queue and returns OK no matter what. The I2C transfer happens from > a thread. > > I wonder if there is precedent, let's ask CAN experts. > > Mark, MCTP would like to report errors from the drivers all the way > to the socket. Do CAN drivers do something along these lines? On CAN_RAW we send fixed size messages (struct can_frame) and there is a bit left to mark a can_frame as an error frame. This basically means we send the error notification inline. What about using sock_queue_err_skb()? We do this in CAN_J1939. regards, Marc
diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c index 4dc057c121f5..e9a835606dfc 100644 --- a/drivers/net/mctp/mctp-i2c.c +++ b/drivers/net/mctp/mctp-i2c.c @@ -485,6 +485,7 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb) struct mctp_i2c_hdr *hdr; struct i2c_msg msg = {0}; u8 *pecp; + struct sock *sk; int rc; fs = mctp_i2c_get_tx_flow_state(midev, skb); @@ -551,6 +552,14 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb) dev_warn_ratelimited(&midev->adapter->dev, "__i2c_transfer failed %d\n", rc); stats->tx_errors++; + + sk = skb->sk; + if (sk) { + sk->sk_err = -rc; + if (!sock_flag(sk, SOCK_DEAD)) + sk_error_report(sk); + } + } else { stats->tx_bytes += skb->len; stats->tx_packets++;
Currently, there is no error handling mechanism for TX failures, causing user space to remain unaware of these failures until a timeout occurs. This leads to unnecessary waiting and delays. This update sends an immediate error notification to user space upon TX failure, reducing wait times and improving response handling for failed tx. Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com> --- drivers/net/mctp/mctp-i2c.c | 9 +++++++++ 1 file changed, 9 insertions(+)