From: Christian Hopps <chopps@labn•net>
To: Sabrina Dubroca <sd@queasysnail•net>
Cc: Christian Hopps <chopps@chopps•org>,
devel@linux-ipsec•org,
Steffen Klassert <steffen.klassert@secunet•com>,
netdev@vger•kernel.org, Christian Hopps <chopps@labn•net>
Subject: Re: [RFC ipsec-next v2 4/8] iptfs: sysctl: allow configuration of global default values
Date: Thu, 01 Feb 2024 08:57:25 -0500 [thread overview]
Message-ID: <m2h6is6zko.fsf@ja.int.chopps.org> (raw)
In-Reply-To: <ZVvmVM3E7dtRK_Ei@hog>
[-- Attachment #1: Type: text/plain, Size: 4750 bytes --]
Sabrina Dubroca <sd@queasysnail•net> writes:
> 2023-11-12, 22:52:15 -0500, Christian Hopps wrote:
>> From: Christian Hopps <chopps@labn•net>
>>
>> Add sysctls for the changing the IPTFS default SA values.
>>
>> Signed-off-by: Christian Hopps <chopps@labn•net>
>> ---
>> Documentation/networking/xfrm_sysctl.rst | 29 ++++++++++++++++++
>> include/net/netns/xfrm.h | 6 ++++
>> include/net/xfrm.h | 7 +++++
>> net/xfrm/xfrm_sysctl.c | 38 ++++++++++++++++++++++++
>> 4 files changed, 80 insertions(+)
>>
>> diff --git a/Documentation/networking/xfrm_sysctl.rst b/Documentation/networking/xfrm_sysctl.rst
>> index 47b9bbdd0179..9e628806c110 100644
>> --- a/Documentation/networking/xfrm_sysctl.rst
>> +++ b/Documentation/networking/xfrm_sysctl.rst
>> @@ -9,3 +9,32 @@ XFRM Syscall
>>
>> xfrm_acq_expires - INTEGER
>> default 30 - hard timeout in seconds for acquire requests
>> +
>> +xfrm_iptfs_maxqsize - UNSIGNED INTEGER
>> + The default IPTFS max output queue size in octets. The output queue is
>> + where received packets destined for output over an IPTFS tunnel are
>> + stored prior to being output in aggregated/fragmented form over the
>> + IPTFS tunnel.
>> +
>> + Default 1M.
>> +
>> +xfrm_iptfs_drptime - UNSIGNED INTEGER
>
> nit: Can we make those names a bit more human-readable?
> xfrm_iptfs_droptime, or possibly xfrm_iptfs_drop_time for better
> consistency with the netlink API.
Changed to xfrm_iptfs_drop_time.
>> + The default IPTFS drop time in microseconds. The drop time is the amount
>> + of time before a missing out-of-order IPTFS tunnel packet is considered
>> + lost. See also the reorder window.
>> +
>> + Default 1s (1000000).
>> +
>> +xfrm_iptfs_idelay - UNSIGNED INTEGER
>
> I would suggest xfrm_iptfs_initial_delay (or at least init_delay like
> the netlink attribute).
Changed to xfrm_iptfs_init_delay.
>
>> + The default IPTFS initial output delay in microseconds. The initial
>> + output delay is the amount of time prior to servicing the output queue
>> + after queueing the first packet on said queue.
>
> Does that also apply if the queue was fully drained (no traffic for a
> while) and starts being used again? That might be worth documenting
> either way (sorry, I haven't processed the main patch far enough to
> answer this question myself yet).
Added: "This applies anytime the output queue was previously empty."
> And it might be worth mentioning that all these sysctls can be
> overridden per SA via the netlink API.
The description of these values as the "default"s implies the fact that they can be changed, I think.
>> + Default 0.
>> +
>> +xfrm_iptfs_rewin - UNSIGNED INTEGER
>
> xfrm_iptfs_reorderwin (or reorder_win, or reorder_window)?
> I'd also make the equivalent netlink attribute's name a bit longer (at
> least spell out REORDER, I can live with WIN for WINDOW).
>
Changed to xfrm_iptfs_reorder_window.
Also renamed the netlink attribute REORD_WIN to match (i.e., not XFRMA_IPTFS_REORDER_WINDOW).
> [...]
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index c9bb0f892f55..d2e87344d175 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -2190,4 +2190,11 @@ static inline int register_xfrm_interface_bpf(void)
>>
>> #endif
>>
>> +#if IS_ENABLED(CONFIG_XFRM_IPTFS)
>> +#define XFRM_IPTFS_DEFAULT_MAX_QUEUE_SIZE (1024 * 1024)
>> +#define XFRM_IPTFS_DEFAULT_INIT_DELAY_USECS (0)
>> +#define XFRM_IPTFS_DEFAULT_DROP_TIME_USECS (1000000)
>> +#define XFRM_IPTFS_DEFAULT_REORDER_WINDOW (3)
>> +#endif
>
> nit: move those to net/xfrm/xfrm_sysctl.c ? they're only used in that file.
Rather than move them directly above where they are only used I just removed them entirely.
Thanks,
Chris.
>> diff --git a/net/xfrm/xfrm_sysctl.c b/net/xfrm/xfrm_sysctl.c
>> index 7fdeafc838a7..bf8e73a6c38e 100644
>> --- a/net/xfrm/xfrm_sysctl.c
>> +++ b/net/xfrm/xfrm_sysctl.c
> [...]
>> @@ -55,6 +87,12 @@ int __net_init xfrm_sysctl_init(struct net *net)
>> table[1].data = &net->xfrm.sysctl_aevent_rseqth;
>> table[2].data = &net->xfrm.sysctl_larval_drop;
>> table[3].data = &net->xfrm.sysctl_acq_expires;
>> +#if IS_ENABLED(CONFIG_XFRM_IPTFS)
>> + table[4].data = &net->xfrm.sysctl_iptfs_drptime;
>> + table[5].data = &net->xfrm.sysctl_iptfs_idelay;
>> + table[6].data = &net->xfrm.sysctl_iptfs_maxqsize;
>> + table[7].data = &net->xfrm.sysctl_iptfs_rewin;
>> +#endif
>
> Is it time to switch to a loop like in ipv6_sysctl_net_init? See
> commit d2f7e56d1e40 ("ipv6: Use math to point per net sysctls into the
> appropriate struct net"). OTOH we don't add sysctls for xfrm very
> often so it's not critical.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 858 bytes --]
next prev parent reply other threads:[~2024-02-01 14:15 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-13 3:52 [RFC ipsec-next v2 0/8] Add IP-TFS mode to xfrm Christian Hopps
2023-11-13 3:52 ` [RFC ipsec-next v2 1/8] iptfs: config: add CONFIG_XFRM_IPTFS Christian Hopps
2023-11-13 3:52 ` [RFC ipsec-next v2 2/8] iptfs: uapi: ip: add ip_tfs_*_hdr packet formats Christian Hopps
2023-11-20 15:28 ` Sabrina Dubroca
2023-11-20 20:18 ` Christian Hopps
2023-11-20 22:38 ` Sabrina Dubroca
2024-02-01 14:15 ` Christian Hopps
2023-11-13 3:52 ` [RFC ipsec-next v2 3/8] iptfs: uapi: IPPROTO_AGGFRAG AGGFRAG in ESP Christian Hopps
2023-11-13 3:52 ` [RFC ipsec-next v2 4/8] iptfs: sysctl: allow configuration of global default values Christian Hopps
2023-11-20 23:05 ` Sabrina Dubroca
2024-02-01 13:57 ` Christian Hopps [this message]
2023-11-13 3:52 ` [RFC ipsec-next v2 5/8] iptfs: netlink: add config (netlink) options Christian Hopps
2023-11-30 15:36 ` Sabrina Dubroca
2023-11-13 3:52 ` [RFC ipsec-next v2 6/8] iptfs: xfrm: Add mode_cbs module functionality Christian Hopps
2023-11-13 14:07 ` [devel-ipsec] " Antony Antony
2023-11-15 19:04 ` Antony Antony
2023-11-30 15:35 ` Sabrina Dubroca
2024-02-01 15:34 ` Christian Hopps
2023-11-13 3:52 ` [RFC ipsec-next v2 7/8] iptfs: xfrm: add generic iptfs defines and functionality Christian Hopps
2023-11-13 3:52 ` [RFC ipsec-next v2 8/8] iptfs: impl: add new iptfs xfrm mode impl Christian Hopps
2023-11-30 15:33 ` Sabrina Dubroca
2024-02-02 9:44 ` Christian Hopps
2023-11-13 7:15 ` [RFC ipsec-next v2 0/8] Add IP-TFS mode to xfrm Steffen Klassert
2023-11-16 15:14 ` [devel-ipsec] " Andrew Cagney
2023-11-20 18:39 ` Christian Hopps
2023-11-20 20:00 ` [DKIM] " Antony Antony
2023-11-20 20:02 ` Antony Antony
2023-11-20 20:14 ` Christian Hopps
2023-11-21 19:13 ` Antony Antony
2023-11-16 16:02 ` Antony Antony
2024-03-07 11:05 ` Christian Hopps
2024-03-10 20:34 ` Antony Antony
2023-11-28 22:12 ` Antony Antony
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=m2h6is6zko.fsf@ja.int.chopps.org \
--to=chopps@labn$(echo .)net \
--cc=chopps@chopps$(echo .)org \
--cc=devel@linux-ipsec$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=sd@queasysnail$(echo .)net \
--cc=steffen.klassert@secunet$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox