| Message ID | 20120420194923.8103.54825.stgit@jf-dev1-dcblab |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > > net/dcb/dcbnl.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c > index 8dfa1da..656c7c7 100644 > --- a/net/dcb/dcbnl.c > +++ b/net/dcb/dcbnl.c > @@ -704,6 +704,7 @@ static int dcbnl_setapp(struct net_device *netdev, struct nlattr **tb, > > ret = dcbnl_reply(err, RTM_SETDCB, DCB_CMD_SAPP, DCB_ATTR_APP, > pid, seq, flags); > + dcbnl_cee_notify(netdev, RTM_SETDCB, DCB_CMD_SAPP, seq, 0); > out: > return ret; > } > @@ -936,6 +937,7 @@ static int dcbnl_setall(struct net_device *netdev, struct nlattr **tb, > > ret = dcbnl_reply(netdev->dcbnl_ops->setall(netdev), RTM_SETDCB, > DCB_CMD_SET_ALL, DCB_ATTR_SET_ALL, pid, seq, flags); > + dcbnl_cee_notify(netdev, RTM_SETDCB, DCB_CMD_SET_ALL, seq, 0); > > return ret; > } > In case of a FW DCBx agent these notification could be a bit confusing. In this case, the dcbnl_setxxx operations are used to set the initial negotiation parameters. dcbnl_setall triggers the negotiation and some time after that the FW DCBx agent driver should send a notification with the newly negotiated values. The notifications sent form within the set operations will return the current negotiated values which may very soon change. If we want to keep the user mode code simple and unified maybe we should send these notifications only if the DCBx agent is host based. Shmulik -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/23/2012 5:51 AM, Shmulik Ravid wrote: >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >> --- >> >> net/dcb/dcbnl.c | 2 ++ >> 1 files changed, 2 insertions(+), 0 deletions(-) >> >> diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c >> index 8dfa1da..656c7c7 100644 >> --- a/net/dcb/dcbnl.c >> +++ b/net/dcb/dcbnl.c >> @@ -704,6 +704,7 @@ static int dcbnl_setapp(struct net_device *netdev, struct nlattr **tb, >> >> ret = dcbnl_reply(err, RTM_SETDCB, DCB_CMD_SAPP, DCB_ATTR_APP, >> pid, seq, flags); >> + dcbnl_cee_notify(netdev, RTM_SETDCB, DCB_CMD_SAPP, seq, 0); >> out: >> return ret; >> } >> @@ -936,6 +937,7 @@ static int dcbnl_setall(struct net_device *netdev, struct nlattr **tb, >> >> ret = dcbnl_reply(netdev->dcbnl_ops->setall(netdev), RTM_SETDCB, >> DCB_CMD_SET_ALL, DCB_ATTR_SET_ALL, pid, seq, flags); >> + dcbnl_cee_notify(netdev, RTM_SETDCB, DCB_CMD_SET_ALL, seq, 0); >> >> return ret; >> } >> > > In case of a FW DCBx agent these notification could be a bit confusing. > In this case, the dcbnl_setxxx operations are used to set the initial > negotiation parameters. dcbnl_setall triggers the negotiation and some > time after that the FW DCBx agent driver should send a notification with > the newly negotiated values. The notifications sent form within the set > operations will return the current negotiated values which may very soon > change. If we want to keep the user mode code simple and unified maybe > we should send these notifications only if the DCBx agent is host based. > > Shmulik > No. We want all the firmware agents and host based agents to look the same from the application. The situation you described is exactly the same for user space as in firmware. The DCBx state machine starts and may call dcbnl_setxxx with initial (local) values. At some later point these may change (possibly because of negotiation with a peer) and we need to call dcbnl_setxxx again. I don't see how this complicates any user mode code? Presumably the agent is listening to DCBx events because it really wants to know the current state of DCBx. It seems to me skipping notifications will actually cause more issues this results in the hardware being in some state that did not trigger any events and the agent will now be out of sync. This is the problem I am trying to solve. btw with this patch we can remove the notify calls in bnx2x. .John -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/23/2012 10:00 AM, Shmulik Ravid wrote: > >> No. We want all the firmware agents and host based agents to look the >> same from the application. The situation you described is exactly the >> same for user space as in firmware. The DCBx state machine starts and may >> call dcbnl_setxxx with initial (local) values. At some later point these >> may change (possibly because of negotiation with a peer) and we need to >> call dcbnl_setxxx again. >> >> I don't see how this complicates any user mode code? Presumably the agent >> is listening to DCBx events because it really wants to know the current >> state of DCBx. It seems to me skipping notifications will actually cause >> more issues this results in the hardware being in some state that did not >> trigger any events and the agent will now be out of sync. This is the >> problem I am trying to solve. >> >> btw with this patch we can remove the notify calls in bnx2x. >> >> .John >> > OK, I see. >>From a user mode application monitoring the netlink notification you get > successive updates each indicating the current valid negotiated > parameters (and HW state) and that's fine. > However I don't see how you can remove the notification call form the > bnx2x. When the FW DCBx agent decides to change the negotiated > parameters (perhaps in response to a peer request), it alerts the driver > which configures the HW and then needs to somehow notify the user about > the newly negotiated parameters. > You are right here. I guess only the notify call after setapp can be removed. Can you ACK the patch to indicate this addresses your concerns? .John > Shmulik > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> No. We want all the firmware agents and host based agents to look the > same from the application. The situation you described is exactly the > same for user space as in firmware. The DCBx state machine starts and may > call dcbnl_setxxx with initial (local) values. At some later point these > may change (possibly because of negotiation with a peer) and we need to > call dcbnl_setxxx again. > > I don't see how this complicates any user mode code? Presumably the agent > is listening to DCBx events because it really wants to know the current > state of DCBx. It seems to me skipping notifications will actually cause > more issues this results in the hardware being in some state that did not > trigger any events and the agent will now be out of sync. This is the > problem I am trying to solve. > > btw with this patch we can remove the notify calls in bnx2x. > > .John > OK, I see. >From a user mode application monitoring the netlink notification you get successive updates each indicating the current valid negotiated parameters (and HW state) and that's fine. However I don't see how you can remove the notification call form the bnx2x. When the FW DCBx agent decides to change the negotiated parameters (perhaps in response to a peer request), it alerts the driver which configures the HW and then needs to somehow notify the user about the newly negotiated parameters. Shmulik -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2012-04-20 at 12:49 -0700, John Fastabend wrote: > This adds code to trigger CEE events when an APP change or setall > command is made from user space. This simplifies user space code > significantly by creating a single interface to listen on that > works with both firmware and userland agents. > > And if we end up with multiple agents this keeps every thing in > sync userland agents, firmware agents, and kernel notifier consumers. > > For an example agent that listens for these events see: > > https://github.com/jrfastab/cgdcbxd > > cgdcbxd is a daemon used to monitor DCB netlink events and manage > the net_prio control group sub-system. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > Acked-by: Shmulik Ravid <shmulikr@broadcom.com> Thanks John, Shmulik -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Shmulik Ravid" <shmulikr@broadcom.com> Date: Tue, 24 Apr 2012 15:56:19 +0300 > > On Fri, 2012-04-20 at 12:49 -0700, John Fastabend wrote: >> This adds code to trigger CEE events when an APP change or setall >> command is made from user space. This simplifies user space code >> significantly by creating a single interface to listen on that >> works with both firmware and userland agents. >> >> And if we end up with multiple agents this keeps every thing in >> sync userland agents, firmware agents, and kernel notifier consumers. >> >> For an example agent that listens for these events see: >> >> https://github.com/jrfastab/cgdcbxd >> >> cgdcbxd is a daemon used to monitor DCB netlink events and manage >> the net_prio control group sub-system. >> >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >> --- >> > > Acked-by: Shmulik Ravid <shmulikr@broadcom.com> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c index 8dfa1da..656c7c7 100644 --- a/net/dcb/dcbnl.c +++ b/net/dcb/dcbnl.c @@ -704,6 +704,7 @@ static int dcbnl_setapp(struct net_device *netdev, struct nlattr **tb, ret = dcbnl_reply(err, RTM_SETDCB, DCB_CMD_SAPP, DCB_ATTR_APP, pid, seq, flags); + dcbnl_cee_notify(netdev, RTM_SETDCB, DCB_CMD_SAPP, seq, 0); out: return ret; } @@ -936,6 +937,7 @@ static int dcbnl_setall(struct net_device *netdev, struct nlattr **tb, ret = dcbnl_reply(netdev->dcbnl_ops->setall(netdev), RTM_SETDCB, DCB_CMD_SET_ALL, DCB_ATTR_SET_ALL, pid, seq, flags); + dcbnl_cee_notify(netdev, RTM_SETDCB, DCB_CMD_SET_ALL, seq, 0); return ret; }
This adds code to trigger CEE events when an APP change or setall command is made from user space. This simplifies user space code significantly by creating a single interface to listen on that works with both firmware and userland agents. And if we end up with multiple agents this keeps every thing in sync userland agents, firmware agents, and kernel notifier consumers. For an example agent that listens for these events see: https://github.com/jrfastab/cgdcbxd cgdcbxd is a daemon used to monitor DCB netlink events and manage the net_prio control group sub-system. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- net/dcb/dcbnl.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html