Date: Thu, 13 Jan 2022 16:27:23 +0100 From: Wojciech Macek <wma@semihalf.com> To: Mark Johnston <markj@freebsd.org> Cc: Wojciech Macek <wma@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-main@freebsd.org Subject: Re: git: 2e72208b6c62 - main - ip_mroute: do not call epoch_waitwhen lock is taken Message-ID: <CANsEV8c_WGG%2BCdtvsAo0PC7qrdBiAMLsWhPwVQ_vmWouhHBHBQ@mail.gmail.com> In-Reply-To: <Yd9K0bT4kgWDHlLd@nuc> References: <202201111019.20BAJjCg058259@gitrepo.freebsd.org> <Yd9K0bT4kgWDHlLd@nuc>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Thanks, i'll take a look if moving mrouter_done outside the lock would work. Wojtek śr., 12 sty 2022, 22:40 użytkownik Mark Johnston <markj@freebsd.org> napisał: > On Tue, Jan 11, 2022 at 10:19:45AM +0000, Wojciech Macek wrote: > > The branch main has been updated by wma: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=2e72208b6c622505323ed48dc58830fc307392b1 > > > > commit 2e72208b6c622505323ed48dc58830fc307392b1 > > Author: Wojciech Macek <wma@FreeBSD.org> > > AuthorDate: 2022-01-11 10:08:35 +0000 > > Commit: Wojciech Macek <wma@FreeBSD.org> > > CommitDate: 2022-01-11 10:19:32 +0000 > > > > ip_mroute: do not call epoch_waitwhen lock is taken > > > > mrouter_done is called with RAW IP lock taken. Some annoying > > printfs are visible on the console if INVARIANTS option is enabled. > > > > Provide atomic-based mechanism which counts enters and exits from/to > > critical section in ip_input and ip_output. > > Before de-initialization of function pointers ensure (with busy-wait) > > that mrouter de-initialization is visible to all readers and that we > don't > > remove pointers (like ip_mforward etc.) in the middle of packet > processing. > > This doesn't address the problem that the warning is complaining about, > which is that a non-sleepable lock is held while performing an expensive > operation. Now we are silently spinning instead, and there's no > guarantee of forward progress since threads may be frequently entering > the MROUTER_RLOCK-protected section in ip_input(). And the change > converts a per-CPU atomic operation (in epoch_enter()) to a global > atomic operation, potentially hurting performance. The atomics are also > missing requisite barriers. > > I think a better solution is to not hold the raw inpcb lock while > calling the ip_mrouter_done() callback. That lock does not provide > synchronization for anything related to ip_mroute.c. Also note that > X_ip_mrouter_done() can sleep in taskqueue_drain(), which is another > reason to avoid holding the lock there. > > > --- > > sys/netinet/ip_mroute.h | 9 +++++---- > > sys/netinet/raw_ip.c | 1 + > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/sys/netinet/ip_mroute.h b/sys/netinet/ip_mroute.h > > index 65c5bdd3a025..faf5b8c72a46 100644 > > --- a/sys/netinet/ip_mroute.h > > +++ b/sys/netinet/ip_mroute.h > > @@ -365,11 +365,12 @@ extern int (*ip_mrouter_set)(struct socket *, > struct sockopt *); > > extern int (*ip_mrouter_get)(struct socket *, struct sockopt *); > > extern int (*ip_mrouter_done)(void); > > extern int (*mrt_ioctl)(u_long, caddr_t, int); > > +extern int ip_mrouter_critical_section_cnt; > > > > -#define MROUTER_RLOCK_TRACKER struct epoch_tracker mrouter_et > > -#define MROUTER_RLOCK() epoch_enter_preempt(net_epoch_preempt, > &mrouter_et) > > -#define MROUTER_RUNLOCK() > epoch_exit_preempt(net_epoch_preempt, &mrouter_et) > > -#define MROUTER_WAIT() epoch_wait_preempt(net_epoch_preempt) > > +#define MROUTER_RLOCK_TRACKER > > Why keep this definition at all? > > > +#define MROUTER_RLOCK() > atomic_add_int(&ip_mrouter_critical_section_cnt, 1) > > +#define MROUTER_RUNLOCK() > atomic_subtract_int(&ip_mrouter_critical_section_cnt, 1) > > +#define MROUTER_WAIT() do {} while > (atomic_load_int(&ip_mrouter_critical_section_cnt) != 0) > > > > #endif /* _KERNEL */ > > > > diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c > > index 7c495745806e..dc49c36f25ad 100644 > > --- a/sys/netinet/raw_ip.c > > +++ b/sys/netinet/raw_ip.c > > @@ -120,6 +120,7 @@ VNET_DEFINE(struct socket *, ip_mrouter); > > int (*ip_mrouter_set)(struct socket *, struct sockopt *); > > int (*ip_mrouter_get)(struct socket *, struct sockopt *); > > int (*ip_mrouter_done)(void); > > +int ip_mrouter_critical_section_cnt; > > int (*ip_mforward)(struct ip *, struct ifnet *, struct mbuf *, > > struct ip_moptions *); > > int (*mrt_ioctl)(u_long, caddr_t, int); > [-- Attachment #2 --] <div dir="auto">Thanks, i'll take a look if moving mrouter_done outside the lock would work.<div dir="auto"><br></div><div dir="auto">Wojtek</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">śr., 12 sty 2022, 22:40 użytkownik Mark Johnston <<a href="mailto:markj@freebsd.org">markj@freebsd.org</a>> napisał:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Jan 11, 2022 at 10:19:45AM +0000, Wojciech Macek wrote:<br> > The branch main has been updated by wma:<br> > <br> > URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=2e72208b6c622505323ed48dc58830fc307392b1" rel="noreferrer noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=2e72208b6c622505323ed48dc58830fc307392b1</a><br>; > <br> > commit 2e72208b6c622505323ed48dc58830fc307392b1<br> > Author: Wojciech Macek <wma@FreeBSD.org><br> > AuthorDate: 2022-01-11 10:08:35 +0000<br> > Commit: Wojciech Macek <wma@FreeBSD.org><br> > CommitDate: 2022-01-11 10:19:32 +0000<br> > <br> > ip_mroute: do not call epoch_waitwhen lock is taken<br> > <br> > mrouter_done is called with RAW IP lock taken. Some annoying<br> > printfs are visible on the console if INVARIANTS option is enabled.<br> > <br> > Provide atomic-based mechanism which counts enters and exits from/to<br> > critical section in ip_input and ip_output.<br> > Before de-initialization of function pointers ensure (with busy-wait)<br> > that mrouter de-initialization is visible to all readers and that we don't<br> > remove pointers (like ip_mforward etc.) in the middle of packet processing.<br> <br> This doesn't address the problem that the warning is complaining about,<br> which is that a non-sleepable lock is held while performing an expensive<br> operation. Now we are silently spinning instead, and there's no<br> guarantee of forward progress since threads may be frequently entering<br> the MROUTER_RLOCK-protected section in ip_input(). And the change<br> converts a per-CPU atomic operation (in epoch_enter()) to a global<br> atomic operation, potentially hurting performance. The atomics are also<br> missing requisite barriers.<br> <br> I think a better solution is to not hold the raw inpcb lock while<br> calling the ip_mrouter_done() callback. That lock does not provide<br> synchronization for anything related to ip_mroute.c. Also note that<br> X_ip_mrouter_done() can sleep in taskqueue_drain(), which is another<br> reason to avoid holding the lock there.<br> <br> > ---<br> > sys/netinet/ip_mroute.h | 9 +++++----<br> > sys/netinet/raw_ip.c | 1 +<br> > 2 files changed, 6 insertions(+), 4 deletions(-)<br> > <br> > diff --git a/sys/netinet/ip_mroute.h b/sys/netinet/ip_mroute.h<br> > index 65c5bdd3a025..faf5b8c72a46 100644<br> > --- a/sys/netinet/ip_mroute.h<br> > +++ b/sys/netinet/ip_mroute.h<br> > @@ -365,11 +365,12 @@ extern int (*ip_mrouter_set)(struct socket *, struct sockopt *);<br> > extern int (*ip_mrouter_get)(struct socket *, struct sockopt *);<br> > extern int (*ip_mrouter_done)(void);<br> > extern int (*mrt_ioctl)(u_long, caddr_t, int);<br> > +extern int ip_mrouter_critical_section_cnt;<br> > <br> > -#define MROUTER_RLOCK_TRACKER struct epoch_tracker mrouter_et<br> > -#define MROUTER_RLOCK() epoch_enter_preempt(net_epoch_preempt, &mrouter_et)<br> > -#define MROUTER_RUNLOCK() epoch_exit_preempt(net_epoch_preempt, &mrouter_et)<br> > -#define MROUTER_WAIT() epoch_wait_preempt(net_epoch_preempt)<br> > +#define MROUTER_RLOCK_TRACKER<br> <br> Why keep this definition at all?<br> <br> > +#define MROUTER_RLOCK() atomic_add_int(&ip_mrouter_critical_section_cnt, 1)<br> > +#define MROUTER_RUNLOCK() atomic_subtract_int(&ip_mrouter_critical_section_cnt, 1)<br> > +#define MROUTER_WAIT() do {} while (atomic_load_int(&ip_mrouter_critical_section_cnt) != 0)<br> > <br> > #endif /* _KERNEL */<br> > <br> > diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c<br> > index 7c495745806e..dc49c36f25ad 100644<br> > --- a/sys/netinet/raw_ip.c<br> > +++ b/sys/netinet/raw_ip.c<br> > @@ -120,6 +120,7 @@ VNET_DEFINE(struct socket *, ip_mrouter);<br> > int (*ip_mrouter_set)(struct socket *, struct sockopt *);<br> > int (*ip_mrouter_get)(struct socket *, struct sockopt *);<br> > int (*ip_mrouter_done)(void);<br> > +int ip_mrouter_critical_section_cnt;<br> > int (*ip_mforward)(struct ip *, struct ifnet *, struct mbuf *,<br> > struct ip_moptions *);<br> > int (*mrt_ioctl)(u_long, caddr_t, int);<br> </blockquote></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANsEV8c_WGG%2BCdtvsAo0PC7qrdBiAMLsWhPwVQ_vmWouhHBHBQ>