| Message ID | 20230317155539.2552954-10-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 403a40f2304d4730a780ab9d6a2b93d1e4ac39d2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: better const qualifier awareness | expand |
On Fri, Mar 17, 2023 at 03:55:38PM +0000, Eric Dumazet wrote: > We can change mptcp_sk() to propagate its argument const qualifier, > thanks to container_of_const(). > > We need to change few things to avoid build errors: > > mptcp_set_datafin_timeout() and mptcp_rtx_head() have to accept > non-const sk pointers. > > @msk local variable in mptcp_pending_tail() must be const. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Matthieu Baerts <matthieu.baerts@tessares.net> Reviewed-by: Simon Horman <simon.horman@corigine.com>
Hi Eric, On 17/03/2023 16:55, Eric Dumazet wrote: > We can change mptcp_sk() to propagate its argument const qualifier, > thanks to container_of_const(). > > We need to change few things to avoid build errors: > > mptcp_set_datafin_timeout() and mptcp_rtx_head() have to accept > non-const sk pointers. > > @msk local variable in mptcp_pending_tail() must be const. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Matthieu Baerts <matthieu.baerts@tessares.net> Good idea! Thank you for this patch and for having Cced me. It looks good to me. I just have one question below if you don't mind. (...) > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index 61fd8eabfca2028680e04558b4baca9f48bbaaaa..4ed8ffffb1ca473179217e640a23bc268742628d 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h (...) > @@ -381,7 +378,7 @@ static inline struct mptcp_data_frag *mptcp_pending_tail(const struct sock *sk) > return list_last_entry(&msk->rtx_queue, struct mptcp_data_frag, list); > } > > -static inline struct mptcp_data_frag *mptcp_rtx_head(const struct sock *sk) > +static inline struct mptcp_data_frag *mptcp_rtx_head(struct sock *sk) It was not clear to me why you had to remove the "const" qualifier here and not just have to add one when assigning the msk just below. But then I looked at what was behind the list_first_entry_or_null() macro used in this function and understood what was the issue. My naive approach would be to modify this macro but I guess we don't want to go down that road, right? -------------------- 8< -------------------- diff --git a/include/linux/list.h b/include/linux/list.h index f10344dbad4d..cd770766f451 100644 --- a/include/linux/list.h +++ b/include/linux/list.h @@ -550,7 +550,7 @@ static inline void list_splice_tail_init(struct list_head *list, * Note that if the list is empty, it returns NULL. */ #define list_first_entry_or_null(ptr, type, member) ({ \ - struct list_head *head__ = (ptr); \ + const struct list_head *head__ = (ptr); \ struct list_head *pos__ = READ_ONCE(head__->next); \ pos__ != head__ ? list_entry(pos__, type, member) : NULL; \ }) -------------------- 8< -------------------- It looks safe to me to do that but I would not trust myself on a Friday evening :) (I'm sure I'm missing something, I'm sorry if it is completely wrong) Anyway if we cannot modify list_first_entry_or_null() one way or another, I'm fine with your modification! Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net> Cheers, Matt > { > struct mptcp_sock *msk = mptcp_sk(sk); >
On Fri, Mar 17, 2023 at 10:32 AM Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > > Hi Eric, > > On 17/03/2023 16:55, Eric Dumazet wrote: > > We can change mptcp_sk() to propagate its argument const qualifier, > > thanks to container_of_const(). > > > > We need to change few things to avoid build errors: > > > > mptcp_set_datafin_timeout() and mptcp_rtx_head() have to accept > > non-const sk pointers. > > > > @msk local variable in mptcp_pending_tail() must be const. > > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Matthieu Baerts <matthieu.baerts@tessares.net> > > Good idea! > > Thank you for this patch and for having Cced me. > > It looks good to me. I just have one question below if you don't mind. > > (...) > > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > > index 61fd8eabfca2028680e04558b4baca9f48bbaaaa..4ed8ffffb1ca473179217e640a23bc268742628d 100644 > > --- a/net/mptcp/protocol.h > > +++ b/net/mptcp/protocol.h > > (...) > > > @@ -381,7 +378,7 @@ static inline struct mptcp_data_frag *mptcp_pending_tail(const struct sock *sk) > > return list_last_entry(&msk->rtx_queue, struct mptcp_data_frag, list); > > } > > > > -static inline struct mptcp_data_frag *mptcp_rtx_head(const struct sock *sk) > > +static inline struct mptcp_data_frag *mptcp_rtx_head(struct sock *sk) > > It was not clear to me why you had to remove the "const" qualifier here > and not just have to add one when assigning the msk just below. But then > I looked at what was behind the list_first_entry_or_null() macro used in > this function and understood what was the issue. > > > My naive approach would be to modify this macro but I guess we don't > want to go down that road, right? > > -------------------- 8< -------------------- > diff --git a/include/linux/list.h b/include/linux/list.h > index f10344dbad4d..cd770766f451 100644 > --- a/include/linux/list.h > +++ b/include/linux/list.h > @@ -550,7 +550,7 @@ static inline void list_splice_tail_init(struct > list_head *list, > * Note that if the list is empty, it returns NULL. > */ > #define list_first_entry_or_null(ptr, type, member) ({ \ > - struct list_head *head__ = (ptr); \ > + const struct list_head *head__ = (ptr); \ > struct list_head *pos__ = READ_ONCE(head__->next); \ > pos__ != head__ ? list_entry(pos__, type, member) : NULL; \ > }) > -------------------- 8< -------------------- This could work, but it is a bit awkward. mptcp_rtx_head() is used in a context where we are changing the socket, not during a readonly lookup ? > > > It looks safe to me to do that but I would not trust myself on a Friday > evening :) > (I'm sure I'm missing something, I'm sorry if it is completely wrong) > > Anyway if we cannot modify list_first_entry_or_null() one way or > another, I'm fine with your modification! > > Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net> > Thanks !
Hi Eric, Thank you for your quick reply! On 17/03/2023 18:47, Eric Dumazet wrote: > On Fri, Mar 17, 2023 at 10:32 AM Matthieu Baerts > <matthieu.baerts@tessares.net> wrote: >> On 17/03/2023 16:55, Eric Dumazet wrote: >> >> (...) >> >>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h >>> index 61fd8eabfca2028680e04558b4baca9f48bbaaaa..4ed8ffffb1ca473179217e640a23bc268742628d 100644 >>> --- a/net/mptcp/protocol.h >>> +++ b/net/mptcp/protocol.h >> >> (...) >> >>> @@ -381,7 +378,7 @@ static inline struct mptcp_data_frag *mptcp_pending_tail(const struct sock *sk) >>> return list_last_entry(&msk->rtx_queue, struct mptcp_data_frag, list); >>> } >>> >>> -static inline struct mptcp_data_frag *mptcp_rtx_head(const struct sock *sk) >>> +static inline struct mptcp_data_frag *mptcp_rtx_head(struct sock *sk) >> >> It was not clear to me why you had to remove the "const" qualifier here >> and not just have to add one when assigning the msk just below. But then >> I looked at what was behind the list_first_entry_or_null() macro used in >> this function and understood what was the issue. >> >> >> My naive approach would be to modify this macro but I guess we don't >> want to go down that road, right? >> >> -------------------- 8< -------------------- >> diff --git a/include/linux/list.h b/include/linux/list.h >> index f10344dbad4d..cd770766f451 100644 >> --- a/include/linux/list.h >> +++ b/include/linux/list.h >> @@ -550,7 +550,7 @@ static inline void list_splice_tail_init(struct >> list_head *list, >> * Note that if the list is empty, it returns NULL. >> */ >> #define list_first_entry_or_null(ptr, type, member) ({ \ >> - struct list_head *head__ = (ptr); \ >> + const struct list_head *head__ = (ptr); \ >> struct list_head *pos__ = READ_ONCE(head__->next); \ >> pos__ != head__ ? list_entry(pos__, type, member) : NULL; \ >> }) >> -------------------- 8< -------------------- > > This could work, but it is a bit awkward. > > mptcp_rtx_head() is used in a context where we are changing the > socket, not during a readonly lookup ? Indeed, you are right. It is currently only used in a context where we are changing the socket. I can see cases where a new packets scheduler might just need to check if the rtx queue is empty but then we should probably add a new helper using list_empty() instead. So yes, no need to change anything! Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 3005a5adf715e8d147c119b0b4c13fcc58fe99f6..8c6b6d2643311b1e30f681e2ae843350342ca6e6 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -459,7 +459,7 @@ static bool mptcp_pending_data_fin(struct sock *sk, u64 *seq) return false; } -static void mptcp_set_datafin_timeout(const struct sock *sk) +static void mptcp_set_datafin_timeout(struct sock *sk) { struct inet_connection_sock *icsk = inet_csk(sk); u32 retransmits; diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 61fd8eabfca2028680e04558b4baca9f48bbaaaa..4ed8ffffb1ca473179217e640a23bc268742628d 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -333,10 +333,7 @@ static inline void msk_owned_by_me(const struct mptcp_sock *msk) sock_owned_by_me((const struct sock *)msk); } -static inline struct mptcp_sock *mptcp_sk(const struct sock *sk) -{ - return (struct mptcp_sock *)sk; -} +#define mptcp_sk(ptr) container_of_const(ptr, struct mptcp_sock, sk.icsk_inet.sk) /* the msk socket don't use the backlog, also account for the bulk * free memory @@ -370,7 +367,7 @@ static inline struct mptcp_data_frag *mptcp_send_next(struct sock *sk) static inline struct mptcp_data_frag *mptcp_pending_tail(const struct sock *sk) { - struct mptcp_sock *msk = mptcp_sk(sk); + const struct mptcp_sock *msk = mptcp_sk(sk); if (!msk->first_pending) return NULL; @@ -381,7 +378,7 @@ static inline struct mptcp_data_frag *mptcp_pending_tail(const struct sock *sk) return list_last_entry(&msk->rtx_queue, struct mptcp_data_frag, list); } -static inline struct mptcp_data_frag *mptcp_rtx_head(const struct sock *sk) +static inline struct mptcp_data_frag *mptcp_rtx_head(struct sock *sk) { struct mptcp_sock *msk = mptcp_sk(sk);
We can change mptcp_sk() to propagate its argument const qualifier, thanks to container_of_const(). We need to change few things to avoid build errors: mptcp_set_datafin_timeout() and mptcp_rtx_head() have to accept non-const sk pointers. @msk local variable in mptcp_pending_tail() must be const. Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Matthieu Baerts <matthieu.baerts@tessares.net> --- net/mptcp/protocol.c | 2 +- net/mptcp/protocol.h | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-)