From: Tejaswi Tanikella <tejaswit@codeaurora•org>
To: Florian Fainelli <f.fainelli@gmail•com>
Cc: netdev@vger•kernel.org, Andrew Lunn <andrew@lunn•ch>
Subject: Re: [PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports
Date: Mon, 4 Jun 2018 20:33:08 +0530 [thread overview]
Message-ID: <20180604150308.GA24669@tejaswit-linux.qualcomm.com> (raw)
In-Reply-To: <8661f50b-876f-50d8-f1a5-61b1dabc5398@gmail.com>
On Fri, Jun 01, 2018 at 07:45:16AM -0700, Florian Fainelli wrote:
Thank you Florian for reviewing my patch.
I had a few questions.
>
>
> On 06/01/2018 07:05 AM, Tejaswi Tanikella wrote:
> > On receiving a IGMPv2/v3 query, based on max_delay set in the header a
> > timer is started to send out a response after a random time within
> > max_delay. If the system then moves into suspend state, Report is
> > delayed until system wakes up.
> >
> > In one reported scenario, on arm64 devices, max_delay was set to 10s,
> > Reports were consistantly delayed if the timer is scheduled after 5 plus
> > seconds.
> >
> > Hold wakelock while starting the timer to prevent moving into suspend
> > state.
>
> I suppose this looks fine, but are not you going to be playing
> whack-a-mole through the network stack wherever there are similar
> patterns? Is not a better solution to move this to
> in_dev_get()/in_dev_put() where you could create a proper wakelock per
> network device?
I see that in_dev_get()/in_dev_put() are being used by some drivers.
won't addition of a wakelock be unnecessary for them?
Will adding two independent functions per in_dev to hold and release
wakelock be sufficient ?
> For instance, I could imagine ARP suffering from the same short comings...
I am not sure if ARP will be effected.
>From my understanding only systems with timers which trigger sending out
responses should be affected. For example IGMP and MLD must send reports
for the router to fwd mcast packets.
In ARP for instance, only moving between states will be delayed. Is there
something that I am missing ?
>
> There is one thing that needs fixing though, see below.
Thanks, I'll fix them in the next patch.
>
> >
> > Signed-off-by: Tejaswi Tanikella <tejaswit@codeaurora•org>
> > ---
> > include/linux/igmp.h | 1 +b
> > net/ipv4/igmp.c | 20 ++++++++++++++++++--
> > 2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/igmp.h b/include/linux/igmp.h
> > index f823185..9be1c58 100644
> > --- a/include/linux/igmp.h
> > +++ b/include/linux/igmp.h
> > @@ -84,6 +84,7 @@ struct ip_mc_list {
> > };
> > struct ip_mc_list __rcu *next_hash;
> > struct timer_list timer;
> > + struct wakeup_source *wakeup_src;
>
> Since you are using this only when CONFIG_IP_MULTICAST is defined, you
> might was well save a few bytes by making this enclosed within an ifdef
> CONFIG_IP_MULTICAST here as well?
>
> [snip]
>
> > @@ -1415,6 +1429,8 @@ void ip_mc_inc_group(struct in_device *in_dev, __be32 addr)
> > #ifdef CONFIG_IP_MULTICAST
> > timer_setup(&im->timer, igmp_timer_expire, 0);
> > im->unsolicit_count = net->ipv4.sysctl_igmp_qrv;
> > + im->wakeup_src = wakeup_source_create("igmp_wakeup_source");
>
> Missing error checking, wakeup_source_create() can return NULL here.
> --
> Florian
next prev parent reply other threads:[~2018-06-04 15:03 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-01 14:05 [PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports Tejaswi Tanikella
2018-06-01 14:45 ` Florian Fainelli
2018-06-04 15:03 ` Tejaswi Tanikella [this message]
2018-06-04 15:06 ` David Miller
2018-06-06 14:38 ` Tejaswi Tanikella
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=20180604150308.GA24669@tejaswit-linux.qualcomm.com \
--to=tejaswit@codeaurora$(echo .)org \
--cc=andrew@lunn$(echo .)ch \
--cc=f.fainelli@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/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