Abhishek Chauhan (ABC) wrote:

On 2/28/2024 12:14 PM, Willem de Bruijn wrote:

quoted

Abhishek Chauhan (ABC) wrote:

quoted

On 2/28/2024 7:53 AM, Willem de Bruijn wrote:

quoted

Abhishek Chauhan wrote:

quoted

Bridge driver today has no support to forward the userspace timestamp
packets and ends up resetting the timestamp. ETF qdisc checks the
packet coming from userspace and encounters to be 0 thereby dropping
time sensitive packets. These changes will allow userspace timestamps
packets to be forwarded from the bridge to NIC drivers.

Existing functionality of mono_delivery_time is not altered here
instead just extended with userspace tstamp support for bridge
forwarding path.

Signed-off-by: Abhishek Chauhan <redacted>
---
Changes since v1 
- Changed the commit subject as i am modifying the mono_delivery_time 
  bit with clockid_delivery_time.
- Took care of suggestion mentioned by Willem to use the same bit for 
  userspace delivery time as there are no conflicts between TCP and 
  SCM_TXTIME, because explicit cmsg makes no sense for TCP and only
  RAW and DGRAM sockets interprets it.

The variable rename churn makes it hard to spot the functional
changes. Perhaps it makes sense just keep the variable name as is,
even though the "mono" is not always technically correct anymore.

  

I think a better approach would be to keep the variable as ease and add
comments and documentation in the header file of skbuff.h like 
how i have done in this patch. The reason why i say this is
a. We can avoid alot of code churn just to solve this basic problem of 
propagating timestamp through forwarding bridge path 
b. Re-use the same variable name and have better documentation 
c. Complexity will be as minimal as possible.

Let me know what you think. 

Agreed

Okay i will make the changes accordingly. 

quoted

quoted

quoted

Or else to split into two patches. One that renames the field.
And one that adds the new behavior of setting the bit for SO_TXTIME.

Regarding the sidenote. I dont see how they are using clock_id to determine 
if the skb->tstamp is set in monotonic. Please correct me or point me to 
the piece of code which is doing so.

That's really out of scope of this series anyway

Sounds good. Really appreciate your review and discussion on this topic. 
 

quoted

quoted

I hope the check against sock_flag is a better implementation as 
it clearly stats and is inline with the implementation that the tstamp is 
coming from userspace. 
skb->mono_delivery_time = sock_flag(sk, SOCK_TXTIME);

Enabling the socket flag is not sufficient to configure a delivery
time on a packet. A transmit time must be communicated per packet
with cork->transmit_time. And on top of that, it is cheaper to test.

So to re-use the same bit of mono_delivery_time. I want to set this bit 
when user-space sets the timestamps using SCM_TXTIME. 
Is it okay if i do the below when we make skb in ipv4/ipv6 and raw packets
to ensure that bridge doesn't reset the packet tstamp or do you have a better 
suggestion to set the bit so br_forward_finish does not reset the timestamp. 

skb->mono_delivery_time = sock_flag(sk, SOCK_TXTIME);

I already gave my suggestion.

The timestamp is passed using sockcm_cookie field transmit_time, which
is set from a control message of type SCM_TXTIME passed to sendmsg.

Right above where you wanted to add this check is where skb->tstamp is
initialized from cork->transmit_time, which got it from this field. So
clearly that is the indication that a transmit time is set. And the
field is hot in the cache.