Re: CVS commit: src/sys/net

[Ryota Ozaki]

On Tue, Feb 16, 2016 at 6:16 AM, Taylor R Campbell
 wrote:
>Date: Mon, 15 Feb 2016 11:06:52 +
>From: Roy Marples 
>
>On 15/02/2016 10:32, Ryota Ozaki wrote:
>> I think we can fix by returning from if_link_state_change_si
>> if ifp->if_link_state == ifp->if_old_link_state. Is it correct?
>
>Then we're not doing what we should because the protocol actions may not
>fire.
>
>Can we not extend softint_schedule to take some user data? That way,
>each call (which I'm assuming would be sequential) can carry the link
>state change in the user data which can then be applied to the interface.
>
> Changing softint is not really a good idea -- it is a significant
> semantic change that most current users don't require, because they
> already have existing queue mechanisms, e.g. pktq.
>
> If it is necessary to report every link state transition, then we need
> to create a queueing mechanism -- and possibly drop some transitions
> in the case of memory shortage, as we already do in route_enqueue.
>
> Maybe something like this, with a queue and a link_state_pending field
> to avoid creating extra queue entries unnecessarily but guaranteeing
> that the most recent link state change will take effect, and be
> preceded by LINK_STATE_DOWN if anything was lost:

Oops. Our (IIJ) local changes have a similar code (softint + queuing)
and it works for long time. So I concur the design.

>
> struct link_state_change {
> SIMPLEQ_ENTRY(link_state_change)lsc_entry;
> int lsc_state;
> boollsc_lost;
> };
>
> static pool_cache_t link_state_change_pc __read_mostly;
>
> void
> if_link_state_change(struct ifnet *ifp, int link_state)
> {
> int s;
>
> s = splnet();
> if (ifp->if_link_state_pending == link_state)
> goto out;
>
> if (ifp->if_link_state_pending != ifp->if_link_state) {

What does the conditional intend?

> struct link_state_change *lsc;
>
> lsc = pool_cache_get(link_state_change_pc, PR_NOWAIT);
> if (lsc == NULL) {
> ifp->if_link_state_lost = true;
> goto change;
> }
>
> lsc->lsc_state = link_state;
> lsc->lsc_lost = ifp->if_link_state_lost;
> SIMPLEQ_INSERT_TAIL(>if_link_state_changes, lsc,
> lsc_entry);
> }
>
> ifp->if_link_state_lost = false;
> change: ifp->if_link_state_pending = link_state;
> softint_schedule(ifp->if_link_state_softint);
> out:splx(s);
> }
>
> static void
> if_link_state_change_softintr(void *cookie)
> {
> struct ifnet *ifp = cookie;
> struct link_state_change *lsc;
> int s;
>
> s = splnet();
> while (!SIMPLEQ_EMPTY(>if_link_state_changes)) {
> lsc = SIMPLEQ_HEAD(>if_link_state_changes);
> SIMPLEQ_REMOVE_HEAD(>if_link_state_changes, lsc_entry);
> if_link_state_change_1(ifp, lsc->lsc_state, lsc->lsc_lost);
> }
>
> if (ifp->if_link_state_pending != ifp->if_link_state)
> if_link_state_change_1(ifp, ifp->if_link_state_pending,
> ifp->if_link_state_lost);
> ifp->if_link_state_lost = false;
> splx(s);
> }
>
> static void
> if_link_state_change_1(struct ifnet *ifp, int link_state, bool lost)
> {
>
> if (lost ||
> (ifp->if_link_state == LINK_STATE_UP &&
> link_state == LINK_STATE_UNKNOWN)) {
> /* pretend LINK_STATE_DOWN first */
> }
>
> ifp->if_link_state = link_state;
> /* call domain dom_if_link_state_change callbacks */
> /* call rt_ifmsg */
> }

Anyway I made a patch based on the above code:
  http://www.netbsd.org/~ozaki-r/link_state_change.diff

What I did are to make your code compilable and workable,
and limit the number of pending events.

Thanks,
  ozaki-r

Re: CVS commit: src/sys/net

[Ryota Ozaki]

On Tue, Feb 16, 2016 at 11:37 AM, matthew green  wrote:
>> Oh, I see. We shouldn't drop any events of link state changes.
>
> i don't see any point in keeping a huge series of link changes
> if they can be collapsed into an equivalent sequence.  with VMs
> and other user-controlled systems it's possible to flood such a
> link state checking engine.

Sorry for my bad wording. I meant we shouldn't drop events _in normal
cases_; the number of events should be several. If a huge number of
events happen in a short period (e.g., due to hardware troubles,
abnormal VMMs, etc.), we should eliminate events to protect the system.

  ozaki-r

re: CVS commit: src/sys/net

[matthew green]

> Oh, I see. We shouldn't drop any events of link state changes.

i don't see any point in keeping a huge series of link changes
if they can be collapsed into an equivalent sequence.  with VMs
and other user-controlled systems it's possible to flood such a
link state checking engine.


.mrg.

netbsd-6 VAX breakage

[John Klos]

Hi,

It seems netbsd-6 VAX is broken:

/home/source/ab/netbsd-6/src/external/bsd/ntp/bin/sntp/../../dist/sntp/libevent/event.c:
 In function 'event_mm_strdup_':
/home/source/ab/netbsd-6/src/external/bsd/ntp/bin/sntp/../../dist/sntp/libevent/event.c:3436:
 warning: comparison is always false due to limited range of data type

from:

http://releng.netbsd.org/builds/netbsd-6/201602132140Z/vax.build.failed


Even if we don't keep snapshots from older builds, it wouldn't hurt to 
keep a few logs. Not sure how long ago this broke, but if I had to 
guess...


http://mail-index.netbsd.org/source-changes/2015/11/07/msg070040.html

John

Re: CVS commit: src/sys/net

[Taylor R Campbell]

   Date: Mon, 15 Feb 2016 11:06:52 +
   From: Roy Marples 

   On 15/02/2016 10:32, Ryota Ozaki wrote:
   > I think we can fix by returning from if_link_state_change_si
   > if ifp->if_link_state == ifp->if_old_link_state. Is it correct?

   Then we're not doing what we should because the protocol actions may not
   fire.

   Can we not extend softint_schedule to take some user data? That way,
   each call (which I'm assuming would be sequential) can carry the link
   state change in the user data which can then be applied to the interface.

Changing softint is not really a good idea -- it is a significant
semantic change that most current users don't require, because they
already have existing queue mechanisms, e.g. pktq.

If it is necessary to report every link state transition, then we need
to create a queueing mechanism -- and possibly drop some transitions
in the case of memory shortage, as we already do in route_enqueue.

Maybe something like this, with a queue and a link_state_pending field
to avoid creating extra queue entries unnecessarily but guaranteeing
that the most recent link state change will take effect, and be
preceded by LINK_STATE_DOWN if anything was lost:

struct link_state_change {
SIMPLEQ_ENTRY(link_state_change)lsc_entry;
int lsc_state;
boollsc_lost;
};

static pool_cache_t link_state_change_pc __read_mostly;

void
if_link_state_change(struct ifnet *ifp, int link_state)
{
int s;

s = splnet();
if (ifp->if_link_state_pending == link_state)
goto out;

if (ifp->if_link_state_pending != ifp->if_link_state) {
struct link_state_change *lsc;

lsc = pool_cache_get(link_state_change_pc, PR_NOWAIT);
if (lsc == NULL) {
ifp->if_link_state_lost = true;
goto change;
}

lsc->lsc_state = link_state;
lsc->lsc_lost = ifp->if_link_state_lost;
SIMPLEQ_INSERT_TAIL(>if_link_state_changes, lsc,
lsc_entry);
}

ifp->if_link_state_lost = false;
change: ifp->if_link_state_pending = link_state;
softint_schedule(ifp->if_link_state_softint);
out:splx(s);
}

static void
if_link_state_change_softintr(void *cookie)
{
struct ifnet *ifp = cookie;
struct link_state_change *lsc;
int s;

s = splnet();
while (!SIMPLEQ_EMPTY(>if_link_state_changes)) {
lsc = SIMPLEQ_HEAD(>if_link_state_changes);
SIMPLEQ_REMOVE_HEAD(>if_link_state_changes, lsc_entry);
if_link_state_change_1(ifp, lsc->lsc_state, lsc->lsc_lost);
}

if (ifp->if_link_state_pending != ifp->if_link_state)
if_link_state_change_1(ifp, ifp->if_link_state_pending,
ifp->if_link_state_lost);
ifp->if_link_state_lost = false;
splx(s);
}

static void
if_link_state_change_1(struct ifnet *ifp, int link_state, bool lost)
{

if (lost ||
(ifp->if_link_state == LINK_STATE_UP &&
link_state == LINK_STATE_UNKNOWN)) {
/* pretend LINK_STATE_DOWN first */
}

ifp->if_link_state = link_state;
/* call domain dom_if_link_state_change callbacks */
/* call rt_ifmsg */
}

Re: CVS commit: src/sys/net

[Martin Husemann]

On Mon, Feb 15, 2016 at 11:06:52AM +, Roy Marples wrote:
> Can we not extend softint_schedule to take some user data? That way,
> each call (which I'm assuming would be sequential) can carry the link
> state change in the user data which can then be applied to the interface.

Sounds like a workqueue(9) then...

Martin

Re: CVS commit: src/sys/net

[Roy Marples]

On 15/02/2016 10:32, Ryota Ozaki wrote:
> On Mon, Feb 15, 2016 at 6:35 PM, Roy Marples  wrote:
>> On 15/02/2016 08:08, Ryota Ozaki wrote:
>>> Module Name:  src
>>> Committed By: ozaki-r
>>> Date: Mon Feb 15 08:08:04 UTC 2016
>>>
>>> Modified Files:
>>>   src/sys/net: if.c if.h
>>>
>>> Log Message:
>>> Run if_link_state_change in softint
>>>
>>> if_link_state_change can execute the network stack that is expected to
>>> not run in hardware interrupt (at least now), however network drivers
>>> may call it in hardware interrupt. Avoid that by introducing a new
>>> softint for if_link_state_change.
>>
>> I don't know anything about softints, so this could be a silly question,
>> but this change looks racey to me. Is there a guarantee anywhere that
>> if_link_state_change() won't be called again before
>> if_link_state_change_si() has been called?
> 
> Not guaranteed. Hmm, you're right. There is a race condition;
> ifp->if_link_state can be overwritten before calling
> if_link_state_change_si and it does matter if ifp->if_link_state
> has been back to ifp->if_old_link_state. (If ifp->if_link_state
> is different from ifp->if_old_link_state, it doesn't matter
> because it's just that the later state wins.)
> 
> I think we can fix by returning from if_link_state_change_si
> if ifp->if_link_state == ifp->if_old_link_state. Is it correct?

Then we're not doing what we should because the protocol actions may not
fire.

Can we not extend softint_schedule to take some user data? That way,
each call (which I'm assuming would be sequential) can carry the link
state change in the user data which can then be applied to the interface.

Roy