* [PATCH] igmp: Make igmp group member RFC 3376 compliant
@ 2016-11-03 9:38 Michal Tesar
2016-11-08 1:13 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Michal Tesar @ 2016-11-03 9:38 UTC (permalink / raw)
To: davem, kuznet, jmorris, kaber, netdev
5.2. Action on Reception of a Query
When a system receives a Query, it does not respond immediately.
Instead, it delays its response by a random amount of time, bounded
by the Max Resp Time value derived from the Max Resp Code in the
received Query message. A system may receive a variety of Queries on
different interfaces and of different kinds (e.g., General Queries,
Group-Specific Queries, and Group-and-Source-Specific Queries), each
of which may require its own delayed response.
Before scheduling a response to a Query, the system must first
consider previously scheduled pending responses and in many cases
schedule a combined response. Therefore, the system must be able to
maintain the following state:
o A timer per interface for scheduling responses to General Queries.
o A per-group and interface timer for scheduling responses to Group-
Specific and Group-and-Source-Specific Queries.
o A per-group and interface list of sources to be reported in the
response to a Group-and-Source-Specific Query.
When a new Query with the Router-Alert option arrives on an
interface, provided the system has state to report, a delay for a
response is randomly selected in the range (0, [Max Resp Time]) where
Max Resp Time is derived from Max Resp Code in the received Query
message. The following rules are then used to determine if a Report
needs to be scheduled and the type of Report to schedule. The rules
are considered in order and only the first matching rule is applied.
1. If there is a pending response to a previous General Query
scheduled sooner than the selected delay, no additional response
needs to be scheduled.
2. If the received Query is a General Query, the interface timer is
used to schedule a response to the General Query after the
selected delay. Any previously pending response to a General
Query is canceled.
--8<--
Currently the timer is rearmed with new random expiration time for
every incoming query regardless of possibly already pending report.
Which is not aligned with the above RFE.
It also might happen that higher rate of incoming queries can
postpone the report after the expiration time of the first query
causing group membership loss.
Now the per interface general query timer is rearmed only
when there is no pending report already scheduled on that interface or
the newly selected expiration time is before the already pending
scheduled report.
Signed-off-by: Michal Tesar <mtesar@redhat.com>
---
net/ipv4/igmp.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 606cc3e..5fc3fd4 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -219,9 +219,14 @@ static void igmp_start_timer(struct ip_mc_list *im, int max_delay)
static void igmp_gq_start_timer(struct in_device *in_dev)
{
int tv = prandom_u32() % in_dev->mr_maxdelay;
+ unsigned long exp = jiffies + tv + 2;
+
+ if (in_dev->mr_gq_running &&
+ time_after_eq(exp, (in_dev->mr_gq_timer).expires))
+ return;
in_dev->mr_gq_running = 1;
- if (!mod_timer(&in_dev->mr_gq_timer, jiffies+tv+2))
+ if (!mod_timer(&in_dev->mr_gq_timer, exp))
in_dev_hold(in_dev);
}
--
2.5.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] igmp: Make igmp group member RFC 3376 compliant
2016-11-03 9:38 [PATCH] igmp: Make igmp group member RFC 3376 compliant Michal Tesar
@ 2016-11-08 1:13 ` David Miller
2016-11-08 9:26 ` Michal Tesar
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2016-11-08 1:13 UTC (permalink / raw)
To: mtesar; +Cc: kuznet, jmorris, kaber, netdev
From: Michal Tesar <mtesar@redhat.com>
Date: Thu, 3 Nov 2016 10:38:34 +0100
> 2. If the received Query is a General Query, the interface timer is
> used to schedule a response to the General Query after the
> selected delay. Any previously pending response to a General
> Query is canceled.
> --8<--
>
> Currently the timer is rearmed with new random expiration time for
> every incoming query regardless of possibly already pending report.
> Which is not aligned with the above RFE.
I don't read it that way. #2 says if this is a general query then any
pending response to a general query is cancelled. And that's
effectively what the code is doing right now.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] igmp: Make igmp group member RFC 3376 compliant
2016-11-08 1:13 ` David Miller
@ 2016-11-08 9:26 ` Michal Tesar
2016-11-16 6:20 ` Hangbin Liu
0 siblings, 1 reply; 7+ messages in thread
From: Michal Tesar @ 2016-11-08 9:26 UTC (permalink / raw)
To: David Miller; +Cc: kuznet, jmorris, kaber, netdev
On Mon, Nov 07, 2016 at 08:13:45PM -0500, David Miller wrote:
> From: Michal Tesar <mtesar@redhat.com>
> Date: Thu, 3 Nov 2016 10:38:34 +0100
>
> > 2. If the received Query is a General Query, the interface timer is
> > used to schedule a response to the General Query after the
> > selected delay. Any previously pending response to a General
> > Query is canceled.
> > --8<--
> >
> > Currently the timer is rearmed with new random expiration time for
> > every incoming query regardless of possibly already pending report.
> > Which is not aligned with the above RFE.
>
> I don't read it that way. #2 says if this is a general query then any
> pending response to a general query is cancelled. And that's
> effectively what the code is doing right now.
Hi David,
I think that it is important to notice that the RFC says also
that only the first matching rule is applied.
"
When new Query with the Router-Alert option arrives on an
interface, provided the system has state to report, a delay for a
response is randomly selected in the range (0, [Max Resp Time]) where
Max Resp Time is derived from Max Resp Code in the received Query
message. The following rules are then used to determine if a Report
needs to be scheduled and the type of Report to schedule. The rules
are considered in order and only the first matching rule is applied.
1. If there is a pending response to a previous General Query
scheduled sooner than the selected delay, no additional response
needs to be scheduled.
2. If the received Query is a General Query, the interface timer is
used to schedule a response to the General Query after the
selected delay. Any previously pending response to a General
Query is canceled.
"
So I would read the above like below:
If some general query arrives and there is some
already pending response scheduled sooner,
no action is needed.
That is how I understand to the rule [1].
But when an general query arrives and there is some other
response already pending, but not sooner as rule one says but later,
new report should be scheduled and the already pending one
needs to be canceled.
That is how I understand to the rule [2]
If there is no already pending report scheduled
the first part of the rule [2] is applied
and new report is scheduled along the selected delay.
So basically we need to compare the already scheduled response exp time
with the one coming in and choose the sooner one.
This is exactly what the patch does.
What do you think?
Best regards Michal Tesar
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] igmp: Make igmp group member RFC 3376 compliant
2016-11-08 9:26 ` Michal Tesar
@ 2016-11-16 6:20 ` Hangbin Liu
2016-11-16 16:19 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Hangbin Liu @ 2016-11-16 6:20 UTC (permalink / raw)
To: Michal Tesar; +Cc: David Miller, kuznet, jmorris, kaber, netdev
Hi David,
On Tue, Nov 08, 2016 at 10:26:25AM +0100, Michal Tesar wrote:
> On Mon, Nov 07, 2016 at 08:13:45PM -0500, David Miller wrote:
>
> > From: Michal Tesar <mtesar@redhat.com>
> > Date: Thu, 3 Nov 2016 10:38:34 +0100
> >
> > > 2. If the received Query is a General Query, the interface timer is
> > > used to schedule a response to the General Query after the
> > > selected delay. Any previously pending response to a General
> > > Query is canceled.
> > > --8<--
> > >
> > > Currently the timer is rearmed with new random expiration time for
> > > every incoming query regardless of possibly already pending report.
> > > Which is not aligned with the above RFE.
> >
> > I don't read it that way. #2 says if this is a general query then any
> > pending response to a general query is cancelled. And that's
> > effectively what the code is doing right now.
>
> Hi David,
> I think that it is important to notice that the RFC says also
> that only the first matching rule is applied.
>
> "
> When new Query with the Router-Alert option arrives on an
> interface, provided the system has state to report, a delay for a
> response is randomly selected in the range (0, [Max Resp Time]) where
> Max Resp Time is derived from Max Resp Code in the received Query
> message. The following rules are then used to determine if a Report
> needs to be scheduled and the type of Report to schedule. The rules
> are considered in order and only the first matching rule is applied.
^^
Would you like to reconsider about this? I also agree with Michal that we
need to choose the sooner timer. Or if we receive query very quickly, we
will keep refresh the timer and may never reply the report.
Thanks
Hangbin
>
> 1. If there is a pending response to a previous General Query
> scheduled sooner than the selected delay, no additional response
> needs to be scheduled.
>
> 2. If the received Query is a General Query, the interface timer is
> used to schedule a response to the General Query after the
> selected delay. Any previously pending response to a General
> Query is canceled.
> "
>
> So I would read the above like below:
> If some general query arrives and there is some
> already pending response scheduled sooner,
> no action is needed.
> That is how I understand to the rule [1].
>
> But when an general query arrives and there is some other
> response already pending, but not sooner as rule one says but later,
> new report should be scheduled and the already pending one
> needs to be canceled.
> That is how I understand to the rule [2]
>
> If there is no already pending report scheduled
> the first part of the rule [2] is applied
> and new report is scheduled along the selected delay.
>
> So basically we need to compare the already scheduled response exp time
> with the one coming in and choose the sooner one.
> This is exactly what the patch does.
>
> What do you think?
> Best regards Michal Tesar
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] igmp: Make igmp group member RFC 3376 compliant
2016-11-16 6:20 ` Hangbin Liu
@ 2016-11-16 16:19 ` David Miller
2016-12-07 12:38 ` Michal Tesar
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2016-11-16 16:19 UTC (permalink / raw)
To: haliu; +Cc: mtesar, kuznet, jmorris, kaber, netdev
From: Hangbin Liu <haliu@redhat.com>
Date: Wed, 16 Nov 2016 14:20:45 +0800
> Hi David,
>
> On Tue, Nov 08, 2016 at 10:26:25AM +0100, Michal Tesar wrote:
>> On Mon, Nov 07, 2016 at 08:13:45PM -0500, David Miller wrote:
>>
>> > From: Michal Tesar <mtesar@redhat.com>
>> > Date: Thu, 3 Nov 2016 10:38:34 +0100
>> >
>> > > 2. If the received Query is a General Query, the interface timer is
>> > > used to schedule a response to the General Query after the
>> > > selected delay. Any previously pending response to a General
>> > > Query is canceled.
>> > > --8<--
>> > >
>> > > Currently the timer is rearmed with new random expiration time for
>> > > every incoming query regardless of possibly already pending report.
>> > > Which is not aligned with the above RFE.
>> >
>> > I don't read it that way. #2 says if this is a general query then any
>> > pending response to a general query is cancelled. And that's
>> > effectively what the code is doing right now.
>>
>> Hi David,
>> I think that it is important to notice that the RFC says also
>> that only the first matching rule is applied.
>>
>> "
>> When new Query with the Router-Alert option arrives on an
>> interface, provided the system has state to report, a delay for a
>> response is randomly selected in the range (0, [Max Resp Time]) where
>> Max Resp Time is derived from Max Resp Code in the received Query
>> message. The following rules are then used to determine if a Report
>> needs to be scheduled and the type of Report to schedule. The rules
>> are considered in order and only the first matching rule is applied.
>
> ^^
>
> Would you like to reconsider about this? I also agree with Michal that we
> need to choose the sooner timer. Or if we receive query very quickly, we
> will keep refresh the timer and may never reply the report.
I'm still thinking about this, please be patient, I review a hundred
patches or more per day so it takes me time to get to tasks that
require deep thinking or real consideration on any level.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] igmp: Make igmp group member RFC 3376 compliant
2016-11-16 16:19 ` David Miller
@ 2016-12-07 12:38 ` Michal Tesar
2016-12-30 3:32 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Michal Tesar @ 2016-12-07 12:38 UTC (permalink / raw)
To: David Miller; +Cc: haliu, kuznet, jmorris, kaber, netdev
On Wed, Nov 16, 2016 at 11:19:43AM -0500, David Miller wrote:
> From: Hangbin Liu <haliu@redhat.com>
> Date: Wed, 16 Nov 2016 14:20:45 +0800
>
> > Hi David,
> >
> > On Tue, Nov 08, 2016 at 10:26:25AM +0100, Michal Tesar wrote:
> >> On Mon, Nov 07, 2016 at 08:13:45PM -0500, David Miller wrote:
> >>
> >> > From: Michal Tesar <mtesar@redhat.com>
> >> > Date: Thu, 3 Nov 2016 10:38:34 +0100
> >> >
> >> > > 2. If the received Query is a General Query, the interface timer is
> >> > > used to schedule a response to the General Query after the
> >> > > selected delay. Any previously pending response to a General
> >> > > Query is canceled.
> >> > > --8<--
> >> > >
> >> > > Currently the timer is rearmed with new random expiration time for
> >> > > every incoming query regardless of possibly already pending report.
> >> > > Which is not aligned with the above RFE.
> >> >
> >> > I don't read it that way. #2 says if this is a general query then any
> >> > pending response to a general query is cancelled. And that's
> >> > effectively what the code is doing right now.
> >>
> >> Hi David,
> >> I think that it is important to notice that the RFC says also
> >> that only the first matching rule is applied.
> >>
> >> "
> >> When new Query with the Router-Alert option arrives on an
> >> interface, provided the system has state to report, a delay for a
> >> response is randomly selected in the range (0, [Max Resp Time]) where
> >> Max Resp Time is derived from Max Resp Code in the received Query
> >> message. The following rules are then used to determine if a Report
> >> needs to be scheduled and the type of Report to schedule. The rules
> >> are considered in order and only the first matching rule is applied.
> >
> > ^^
> >
> > Would you like to reconsider about this? I also agree with Michal that we
> > need to choose the sooner timer. Or if we receive query very quickly, we
> > will keep refresh the timer and may never reply the report.
>
> I'm still thinking about this, please be patient, I review a hundred
> patches or more per day so it takes me time to get to tasks that
> require deep thinking or real consideration on any level.
Hi Dave,
would it be possible to have another look at this patch and reconsider
its behavior? I really believe that current code does not behave
correctly.
Best regards Michal Tesar
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-12-30 3:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-03 9:38 [PATCH] igmp: Make igmp group member RFC 3376 compliant Michal Tesar
2016-11-08 1:13 ` David Miller
2016-11-08 9:26 ` Michal Tesar
2016-11-16 6:20 ` Hangbin Liu
2016-11-16 16:19 ` David Miller
2016-12-07 12:38 ` Michal Tesar
2016-12-30 3:32 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).