| Message ID | 20231113035219.920136-3-chopps@chopps.org (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Add IP-TFS mode to xfrm | expand |
2023-11-12, 22:52:13 -0500, Christian Hopps wrote: > From: Christian Hopps <chopps@labn.net> > > Add the on-wire basic and congestion-control IP-TFS packet headers. > > Signed-off-by: Christian Hopps <chopps@labn.net> > --- > include/uapi/linux/ip.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h > index 283dec7e3645..cc83878ecf08 100644 > --- a/include/uapi/linux/ip.h > +++ b/include/uapi/linux/ip.h > @@ -137,6 +137,23 @@ struct ip_beet_phdr { > __u8 reserved; > }; > > +struct ip_iptfs_hdr { > + __u8 subtype; /* 0*: basic, 1: CC */ > + __u8 flags; > + __be16 block_offset; > +}; > + > +struct ip_iptfs_cc_hdr { > + __u8 subtype; /* 0: basic, 1*: CC */ > + __u8 flags; > + __be16 block_offset; > + __be32 loss_rate; > + __u8 rtt_and_adelay1[4]; > + __u8 adelay2_and_xdelay[4]; Given how the fields are split, wouldn't it be more convenient to have a single __be64, rather than reading some bits from multiple __u8? > + __be32 tval; > + __be32 techo; > +}; I don't think these need to be part of uapi. Can we move them to include/net/iptfs.h (or possibly net/xfrm/xfrm_iptfs.c)? It would also make more sense to have them near the definitions for IPTFS_SUBTYPE_*. And it would be easier to change how we split and name fields for kernel consumption if we're not stuck with whatever we put in uapi.
Sabrina Dubroca <sd@queasysnail.net> writes: > 2023-11-12, 22:52:13 -0500, Christian Hopps wrote: >> From: Christian Hopps <chopps@labn.net> >> >> Add the on-wire basic and congestion-control IP-TFS packet headers. >> >> Signed-off-by: Christian Hopps <chopps@labn.net> >> --- >> include/uapi/linux/ip.h | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h >> index 283dec7e3645..cc83878ecf08 100644 >> --- a/include/uapi/linux/ip.h >> +++ b/include/uapi/linux/ip.h >> @@ -137,6 +137,23 @@ struct ip_beet_phdr { >> __u8 reserved; >> }; >> >> +struct ip_iptfs_hdr { >> + __u8 subtype; /* 0*: basic, 1: CC */ >> + __u8 flags; >> + __be16 block_offset; >> +}; >> + >> +struct ip_iptfs_cc_hdr { >> + __u8 subtype; /* 0: basic, 1*: CC */ >> + __u8 flags; >> + __be16 block_offset; >> + __be32 loss_rate; >> + __u8 rtt_and_adelay1[4]; >> + __u8 adelay2_and_xdelay[4]; > > Given how the fields are split, wouldn't it be more convenient to have > a single __be64, rather than reading some bits from multiple __u8? This is a good point, I carried this over from an earlier implementation, let me give it some though but probably change it. >> + __be32 tval; >> + __be32 techo; >> +}; > I don't think these need to be part of uapi. Can we move them to > include/net/iptfs.h (or possibly net/xfrm/xfrm_iptfs.c)? It would also > make more sense to have them near the definitions for > IPTFS_SUBTYPE_*. And it would be easier to change how we split and > name fields for kernel consumption if we're not stuck with whatever we > put in uapi. The other ipsec modes headers were added here, so I was simply following along. I don't mind moving things but would like to understand why iptfs would be different from the other modes, for example, `struct ip_comp_hdr` and `struct ip_beet_phdr` appears in this file. Thanks! Chris.
2023-11-20, 15:18:49 -0500, Christian Hopps wrote: > > Sabrina Dubroca <sd@queasysnail.net> writes: > > > 2023-11-12, 22:52:13 -0500, Christian Hopps wrote: > > > From: Christian Hopps <chopps@labn.net> > > > > > > Add the on-wire basic and congestion-control IP-TFS packet headers. > > > > > > Signed-off-by: Christian Hopps <chopps@labn.net> > > > --- > > > include/uapi/linux/ip.h | 17 +++++++++++++++++ > > > 1 file changed, 17 insertions(+) > > > > > > diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h > > > index 283dec7e3645..cc83878ecf08 100644 > > > --- a/include/uapi/linux/ip.h > > > +++ b/include/uapi/linux/ip.h > > > @@ -137,6 +137,23 @@ struct ip_beet_phdr { > > > __u8 reserved; > > > }; > > > > > > +struct ip_iptfs_hdr { > > > + __u8 subtype; /* 0*: basic, 1: CC */ > > > + __u8 flags; > > > + __be16 block_offset; > > > +}; > > > + > > > +struct ip_iptfs_cc_hdr { > > > + __u8 subtype; /* 0: basic, 1*: CC */ > > > + __u8 flags; > > > + __be16 block_offset; > > > + __be32 loss_rate; > > > + __u8 rtt_and_adelay1[4]; > > > + __u8 adelay2_and_xdelay[4]; > > > > Given how the fields are split, wouldn't it be more convenient to have > > a single __be64, rather than reading some bits from multiple __u8? > > This is a good point, I carried this over from an earlier implementation, let me give it some though but probably change it. > > > > + __be32 tval; > > > + __be32 techo; > > > +}; > > > I don't think these need to be part of uapi. Can we move them to > > include/net/iptfs.h (or possibly net/xfrm/xfrm_iptfs.c)? It would also > > make more sense to have them near the definitions for > > IPTFS_SUBTYPE_*. And it would be easier to change how we split and > > name fields for kernel consumption if we're not stuck with whatever we > > put in uapi. > > The other ipsec modes headers were added here, so I was simply > following along. I don't mind moving things but would like to > understand why iptfs would be different from the other modes, for > example, `struct ip_comp_hdr` and `struct ip_beet_phdr` appears in > this file. IMHO it was a mistake that was made when include/uapi was created, they should not have been part of uapi since there's never an exchange between kernel and userspace using those. I guess it's less problematic because the header formats are simple (everything fits nicely into a u8/u16/u32) and they were already used in the kernel for a long time so pretty much stable (ie no doubt about whether the split and naming that was chosen was right). But if others feel strongly about putting those structs in uapi, I can live with that. I'll send comments on the rest of the series as I continue making my way through it. The main patch is going to take me a while :(
Sabrina Dubroca <sd@queasysnail.net> writes: > 2023-11-12, 22:52:13 -0500, Christian Hopps wrote: >> From: Christian Hopps <chopps@labn.net> >> >> Add the on-wire basic and congestion-control IP-TFS packet headers. >> >> Signed-off-by: Christian Hopps <chopps@labn.net> >> --- >> include/uapi/linux/ip.h | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h >> index 283dec7e3645..cc83878ecf08 100644 >> --- a/include/uapi/linux/ip.h >> +++ b/include/uapi/linux/ip.h >> @@ -137,6 +137,23 @@ struct ip_beet_phdr { >> __u8 reserved; >> }; >> >> +struct ip_iptfs_hdr { >> + __u8 subtype; /* 0*: basic, 1: CC */ >> + __u8 flags; >> + __be16 block_offset; >> +}; >> + >> +struct ip_iptfs_cc_hdr { >> + __u8 subtype; /* 0: basic, 1*: CC */ >> + __u8 flags; >> + __be16 block_offset; >> + __be32 loss_rate; >> + __u8 rtt_and_adelay1[4]; >> + __u8 adelay2_and_xdelay[4]; > > Given how the fields are split, wouldn't it be more convenient to have > a single __be64, rather than reading some bits from multiple __u8? Changed this to __be64. >> + __be32 tval; >> + __be32 techo; >> +}; > > I don't think these need to be part of uapi. Can we move them to > include/net/iptfs.h (or possibly net/xfrm/xfrm_iptfs.c)? It would also > make more sense to have them near the definitions for > IPTFS_SUBTYPE_*. And it would be easier to change how we split and > name fields for kernel consumption if we're not stuck with whatever we > put in uapi. I saw this also as the place documenting the packet format. Userland can have raw packets delivered to them... Mostly though I was following the pattern that existed already. Thanks, Chris.
diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h index 283dec7e3645..cc83878ecf08 100644 --- a/include/uapi/linux/ip.h +++ b/include/uapi/linux/ip.h @@ -137,6 +137,23 @@ struct ip_beet_phdr { __u8 reserved; }; +struct ip_iptfs_hdr { + __u8 subtype; /* 0*: basic, 1: CC */ + __u8 flags; + __be16 block_offset; +}; + +struct ip_iptfs_cc_hdr { + __u8 subtype; /* 0: basic, 1*: CC */ + __u8 flags; + __be16 block_offset; + __be32 loss_rate; + __u8 rtt_and_adelay1[4]; + __u8 adelay2_and_xdelay[4]; + __be32 tval; + __be32 techo; +}; + /* index values for the variables in ipv4_devconf */ enum {