* Re: [scsi/net-next]Pull csiostor from net-next
From: hch @ 2014-12-30 9:36 UTC (permalink / raw)
To: Praveen Madhavan; +Cc: linux-scsi@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <24C222ADD7729F4FB5113B41FF0028A1BCC18E@nice.asicdesigners.com>
On Sun, Dec 28, 2014 at 03:13:14PM +0000, Praveen Madhavan wrote:
> > How much do you plan to send for the 3.20 window? I'd rather avoid
> > having to pull in the net-next tree and merge everything through Dave
> > if that seems feasible.
> I hv couple of patches fixes for 3.20 window. Can you please pull from linux-next tree ?
If it's really just a few fixes I'd prefer to just ACK them on the scsi
list and send them through the net tree to avoid a cross dependency.
^ permalink raw reply
* [PATCH 2/3][v2] net/fsl: remove irq assignment from xgmac_mdio
From: shh.xie @ 2014-12-30 8:28 UTC (permalink / raw)
To: netdev, davem; +Cc: Shaohui Xie
From: Shaohui Xie <Shaohui.Xie@freescale.com>
Which is wrong and not used, so no extra space needed by
mdiobus_alloc_size(), use mdiobus_alloc() instead.
Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
---
changes in v2:
use mdiobus_alloc() instead of mdiobus_alloc_size(0).
drivers/net/ethernet/freescale/xgmac_mdio.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 90adba1..72e0b85 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -187,14 +187,13 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
return ret;
}
- bus = mdiobus_alloc_size(PHY_MAX_ADDR * sizeof(int));
+ bus = mdiobus_alloc();
if (!bus)
return -ENOMEM;
bus->name = "Freescale XGMAC MDIO Bus";
bus->read = xgmac_mdio_read;
bus->write = xgmac_mdio_write;
- bus->irq = bus->priv;
bus->parent = &pdev->dev;
snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res.start);
--
1.8.4.1
^ permalink raw reply related
* [PATCH 1/3][v2] net/fsl: remove reset from xgmac_mdio
From: shh.xie @ 2014-12-30 8:27 UTC (permalink / raw)
To: netdev, davem; +Cc: Shaohui Xie
From: Shaohui Xie <Shaohui.Xie@freescale.com>
Since the reset is just clock setting, individual mdio reset is
not available.
Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
---
changes in v2:
no change.
drivers/net/ethernet/freescale/xgmac_mdio.c | 19 -------------------
1 file changed, 19 deletions(-)
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 6e7db66..90adba1 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -174,24 +174,6 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
return value;
}
-/* Reset the MIIM registers, and wait for the bus to free */
-static int xgmac_mdio_reset(struct mii_bus *bus)
-{
- struct tgec_mdio_controller __iomem *regs = bus->priv;
- int ret;
-
- mutex_lock(&bus->mdio_lock);
-
- /* Setup the MII Mgmt clock speed */
- out_be32(®s->mdio_stat, MDIO_STAT_CLKDIV(100));
-
- ret = xgmac_wait_until_free(&bus->dev, regs);
-
- mutex_unlock(&bus->mdio_lock);
-
- return ret;
-}
-
static int xgmac_mdio_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
@@ -212,7 +194,6 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
bus->name = "Freescale XGMAC MDIO Bus";
bus->read = xgmac_mdio_read;
bus->write = xgmac_mdio_write;
- bus->reset = xgmac_mdio_reset;
bus->irq = bus->priv;
bus->parent = &pdev->dev;
snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res.start);
--
1.8.4.1
^ permalink raw reply related
* [Question] What's the noop_qdisc introduced for in the kernel?
From: Dennis Chen @ 2014-12-30 9:23 UTC (permalink / raw)
To: netdev
After google and the code reading, seems this Qdisc instance is only
used for the initialization purpose, I can't find the reason that this
object introduced in the kernel. Does anybody know what the historical
reason is for this invention? the purpose or the benefit for this
Qdisc object?
--
Den
^ permalink raw reply
* RE: [PATCH] qlcnic: Fix return value in qlcnic_probe()
From: Shahed Shaikh @ 2014-12-30 8:45 UTC (permalink / raw)
To: xuyongjiande@163.com, Dept-GE Linux NIC Dev
Cc: netdev, linux-kernel, Yongjian Xu
In-Reply-To: <1419926626-26624-1-git-send-email-xuyongjiande@163.com>
> -----Original Message-----
> From: xuyongjiande@163.com [mailto:xuyongjiande@163.com]
> Sent: Tuesday, December 30, 2014 1:34 PM
> To: Shahed Shaikh; Dept-GE Linux NIC Dev
> Cc: netdev; linux-kernel; Yongjian Xu
> Subject: [PATCH] qlcnic: Fix return value in qlcnic_probe()
>
> From: Yongjian Xu <xuyongjiande@gmail.com>
>
> If the check of adapter fails and goes into the 'else' branch, the return value
> 'err' should not still be zero.
>
> Signed-off-by: Yongjian Xu <xuyongjiande@gmail.com>
> ---
> drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Shahed Shaikh <shahed.shaikh@qlogic.com>
Thanks,
Shahed
^ permalink raw reply
* RE: [PATCH 3/6] bridge: modify IFLA_AF_SPEC parser to parse IFLA_BRIDGE_VLAN_RANGE_INFO
From: Arad, Ronen @ 2014-12-30 8:40 UTC (permalink / raw)
To: roopa@cumulusnetworks.com, netdev@vger.kernel.org,
shemminger@vyatta.com, vyasevic@redhat.com
Cc: Wilson kok
In-Reply-To: <1419887132-7084-4-git-send-email-roopa@cumulusnetworks.com>
>-----Original Message-----
>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
>Behalf Of roopa@cumulusnetworks.com
>Sent: Monday, December 29, 2014 11:05 PM
>To: netdev@vger.kernel.org; shemminger@vyatta.com; vyasevic@redhat.com
>Cc: Roopa Prabhu; Wilson kok
>Subject: [PATCH 3/6] bridge: modify IFLA_AF_SPEC parser to parse
>IFLA_BRIDGE_VLAN_RANGE_INFO
>
>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
>This patch modifies br_afspec to parse incoming IFLA_BRIDGE_VLAN_RANGE_INFO
>
>Signed-off-by: Wilson kok <wkok@cumulusnetworks.com>
>Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>---
> net/bridge/br_netlink.c | 70 +++++++++++++++++++++++++++++++++-------------
>-
> 1 file changed, 49 insertions(+), 21 deletions(-)
>
>diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>index e7d1fc0..4c47ba0 100644
>--- a/net/bridge/br_netlink.c
>+++ b/net/bridge/br_netlink.c
>@@ -227,48 +227,76 @@ static const struct nla_policy
>ifla_br_policy[IFLA_MAX+1] = {
> .len = sizeof(struct bridge_vlan_range_info), },
> };
>
>+static int br_afspec_vlan_add(struct net_bridge *br,
>+ struct net_bridge_port *p,
>+ u16 vid, u16 flags)
>+{
>+ int err = 0;
>+
>+ if (p) {
>+ err = nbp_vlan_add(p, vid, flags);
>+ if (err)
>+ return err;
>+
>+ if (flags & BRIDGE_VLAN_INFO_MASTER)
>+ err = br_vlan_add(p->br, vid, flags);
>+ } else {
>+ err = br_vlan_add(br, vid, flags);
>+ }
>+
>+ return err;
>+}
>+
>+static void br_afspec_vlan_del(struct net_bridge *br,
>+ struct net_bridge_port *p,
>+ u16 vid, u16 flags)
>+{
>+ if (p) {
>+ nbp_vlan_delete(p, vid);
>+ if (flags & BRIDGE_VLAN_INFO_MASTER)
>+ br_vlan_delete(p->br, vid);
>+ } else {
>+ br_vlan_delete(br, vid);
>+ }
>+}
>+
> static int br_afspec(struct net_bridge *br,
> struct net_bridge_port *p,
> struct nlattr *af_spec,
> int cmd)
> {
>- struct bridge_vlan_info *vinfo;
>- int err = 0;
>+ struct bridge_vlan_range_info *vinfo;
> struct nlattr *attr;
> int err = 0;
> int rem;
> u16 vid;
>
> nla_for_each_nested(attr, af_spec, rem) {
>- if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
>+ if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO &&
>+ nla_type(attr) != IFLA_BRIDGE_VLAN_RANGE_INFO)
> continue;
>-
> vinfo = nla_data(attr);
>- if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK)
>+
>+ if (nla_type(attr) == IFLA_BRIDGE_VLAN_INFO)
>+ vinfo->vid_end = vinfo->vid;
>+
>+ if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK ||
>+ vinfo->vid_end >= VLAN_VID_MASK ||
>+ vinfo->vid > vinfo->vid_end)
> return -EINVAL;
>
> switch (cmd) {
> case RTM_SETLINK:
>- if (p) {
>- err = nbp_vlan_add(p, vinfo->vid, vinfo->flags);
>+ for (vid = vinfo->vid; vid <= vinfo->vid_end; vid++) {
>+ err = br_afspec_vlan_add(br, p, vid,
>+ vinfo->flags);
vinfo->flags could have BRIDGE_VLAN_INFO_PVID set. It is really a port property and there could only be a single PVID for a port. The loop will make the port pvid set, in turn, to each vid in the range until it finally set to vid_end.
This could be avoided by turning off this flag for all but the last vid in range:
err = br_afspec_vlan_addr(br, p, vid,
((vid == vinfo->vid_end)
? vinfo->flags
: vinfo->flags & ~BRIDGE_VLAN_INFO_PVID));
Another alternative could be to add explicit pvid field to bridge_vlan_range_info and disallow PVID flag.
This allows for setting the port pvid to any vid in the range.
If both alternatives seem somewhat complex we could just disallow PVID flag in IFLA_BRIDGE_VLAN_RANGE_INFO and allow it only in IFLA_BRIDGE_VLAN_INFO.
> if (err)
> break;
>-
>- if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER)
>- err = br_vlan_add(p->br, vinfo->vid,
>- vinfo->flags);
>- } else
>- err = br_vlan_add(br, vinfo->vid, vinfo->flags);
>-
>+ }
> break;
>-
> case RTM_DELLINK:
>- if (p) {
>- nbp_vlan_delete(p, vinfo->vid);
>- if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER)
>- br_vlan_delete(p->br, vinfo->vid);
>- } else
>- br_vlan_delete(br, vinfo->vid);
>+ for (vid = vinfo->vid; vid <= vinfo->vid_end; vid++)
>+ br_afspec_vlan_del(br, p, vid, vinfo->flags);
> break;
> }
> }
>--
>1.7.10.4
>
>--
>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
^ permalink raw reply
* Re: [PATCH] bonding: move ipoib_header_ops to vmlinux
From: Wengang @ 2014-12-30 8:26 UTC (permalink / raw)
To: David Miller; +Cc: cwang, netdev, linux-rdma
In-Reply-To: <20141229.232507.2086816594770925202.davem@davemloft.net>
于 2014年12月30日 12:25, David Miller 写道:
> From: Wengang <wen.gang.wang@oracle.com>
> Date: Tue, 30 Dec 2014 11:01:42 +0800
>
>> There are more than one way we do things. For this case, considering
>> needs, complexity and stability I think moving ipoib_header_ops is the
>> right way to go.
> I completely disagree, it's a gross hack at best.
>
> It's papering over the real problem.
>
> When we have references to released objects in other areas of the
> networking stack, we don't move those objects into the static kernel
> image as a fix.
OK. Let me see if I can make a patch to match what you want.
Thanks
wengang
^ permalink raw reply
* Re: [PATCH 5/6] bridge: new function to pack vlans using both IFLA_BRIDGE_VLAN_INFO and IFLA_BRIDGE_VLAN_RANGE_INFO
From: Scott Feldman @ 2014-12-30 8:04 UTC (permalink / raw)
To: roopa; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson kok
In-Reply-To: <54A238BD.7050707@cumulusnetworks.com>
On Mon, Dec 29, 2014 at 9:31 PM, roopa <roopa@cumulusnetworks.com> wrote:
> On 12/29/14, 3:25 PM, Scott Feldman wrote:
>>
>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>>>
>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>>> This patch adds new function to compress vlans into ranges.
>>> Vlans are compressed into ranges only if the fill request is called with
>>> RTEXT_FILTER_BRVLAN_COMPRESSED in filtermask.
>>>
>>> Old vlan packing code is moved to a new function and continues to be
>>> called when filter_mask is RTEXT_FILTER_BRVLAN
>>>
>>> Signed-off-by: Wilson kok <wkok@cumulusnetworks.com>
>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>> ---
>>> net/bridge/br_netlink.c | 157
>>> +++++++++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 137 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>> index 4c47ba0..16bdd5a 100644
>>> --- a/net/bridge/br_netlink.c
>>> +++ b/net/bridge/br_netlink.c
>>> @@ -67,6 +67,133 @@ static int br_port_fill_attrs(struct sk_buff *skb,
>>> return 0;
>>> }
>>>
>>> +static int br_fill_ifvlaninfo_bitmap(struct sk_buff *skb,
>>> + const unsigned long *vlan_bmp, u16
>>> flags)
>>> +{
>>> + struct bridge_vlan_range_info vinfo_range;
>>> + struct bridge_vlan_info vinfo;
>>> + u16 vid, start = 0, end = 0;
>>
>> One per line?
>
> ack
>
>>> + u16 pvid;
>>
>> Aren't you getting an "unused" compile warning on this ^^^?
>
> will double check..
>
>>> +
>>> + /* handle the untagged */
>>
>> This comment ^^^ doesn't make sense here.
>>
>>> + for_each_set_bit(vid, vlan_bmp, VLAN_N_VID) {
>>> + if (start == 0) {
>>> + start = vid;
>>> + end = vid;
>>> + }
>>> + if ((vid - end) > 1) {
>>> + memset(&vinfo_range, 0, sizeof(vinfo_range));
>>> + vinfo_range.flags |= flags;
>>> + vinfo_range.vid = start;
>>> + vinfo_range.vid_end = end;
>>> + if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO,
>>> + sizeof(vinfo_range), &vinfo_range))
>>> + goto nla_put_failure;
>>> + start = vid;
>>> + end = vid;
>>> + } else {
>>> + end = vid;
>>> + }
>>> + }
>>
>> What happens with this set {1-10, 12, 20-25, 30}? On vid 12, will
>> that put a VLAN_RANGE_INFO with start=end=12? Seems strange to use
>> RANGE_INFO for single vlan.
>>
>> Can the algorithm be simplified? Maybe there are other examples in
>> the kernel of finding ranges/singles from a bitmap we could borrow
>> code from?
>
> let me see...
>
>>> + if (start != 0 && end != 0) {
>>> + if (start != end) {
>>> + memset(&vinfo_range, 0, sizeof(vinfo_range));
>>> + vinfo_range.flags |= flags;
>>> + vinfo_range.vid = start;
>>> + vinfo_range.vid_end = end;
>>> + if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO,
>>> + sizeof(vinfo_range), &vinfo_range))
>>> + goto nla_put_failure;
>>> + } else {
>>> + memset(&vinfo, 0, sizeof(vinfo));
>>> + vinfo.flags |= flags;
>>> + vinfo.vid = start;
>>> + if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
>>> + sizeof(vinfo), &vinfo))
>>> + goto nla_put_failure;
>>
>> How many IFLA_BRIDGE_VLAN_INFOs can you fit into skb? It seems the
>> worst case is you'll have 4094/2 = 2047 VLAN_INFOs if using all
>> even-numbered vids, for example. Will that fit? I guess the original
>> code had the same concern...wonder if anyone checked this worst-case?
>
>
> I will check this again. Remember doing worst case tests for setlinks. will
> get back with some worst cases tests in the next submission.
>>
>>
>>> + }
>>> + }
>>> +
>>> +nla_put_failure:
>>> + return -EMSGSIZE;
>>> +}
>>> +
>>> +static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb,
>>> + const struct net_port_vlans *pv)
>>> +{
>>> + unsigned long vlan_bmp_copy[BR_VLAN_BITMAP_LEN];
>>> + unsigned long untagged_bmp_copy[BR_VLAN_BITMAP_LEN];
>>
>> Lots of automatic space on the stack...can you use dynamic mem
>> (kzalloc) for these?
>
> Let me see. Or make it a static global ?
Static global would be bad news for multiple threads doing a vlan fill.
^ permalink raw reply
* [PATCH] qlcnic: Fix return value in qlcnic_probe()
From: xuyongjiande @ 2014-12-30 8:03 UTC (permalink / raw)
To: shahed.shaikh, Dept-GELinuxNICDev; +Cc: netdev, linux-kernel, Yongjian Xu
From: Yongjian Xu <xuyongjiande@gmail.com>
If the check of adapter fails and goes into the 'else' branch, the
return value 'err' should not still be zero.
Signed-off-by: Yongjian Xu <xuyongjiande@gmail.com>
---
drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 1aa25b1..a95b604 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -2603,6 +2603,7 @@ qlcnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
} else {
dev_err(&pdev->dev,
"%s: failed. Please Reboot\n", __func__);
+ err = -ENODEV;
goto err_out_free_hw;
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 3/3] net: ieee802154: don't use devm_pinctrl_get_select_default() in probe
From: Marcel Holtmann @ 2014-12-30 6:17 UTC (permalink / raw)
To: Wolfram Sang
Cc: Linux Kernel Mailing List, linux-gpio, Varka Bhadram,
Alexander Aring, linux-wpan, netdev
In-Reply-To: <1419286616-9893-3-git-send-email-wsa@the-dreams.de>
Hi Wolfram,
> Since commit ab78029ecc34 (drivers/pinctrl: grab default handles from device
> core), we can rely on device core for setting the default pins.
>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
> drivers/net/ieee802154/cc2520.c | 7 -------
> 1 file changed, 7 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC
From: roopa @ 2014-12-30 6:01 UTC (permalink / raw)
To: Scott Feldman; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok
In-Reply-To: <CAE4R7bDjmHrW+bzoPJ-fpN51ykTnqjrXy7hCqXgJNv-EHayiHg@mail.gmail.com>
On 12/29/14, 9:31 PM, Scott Feldman wrote:
> On Mon, Dec 29, 2014 at 9:25 PM, roopa <roopa@cumulusnetworks.com> wrote:
>> On 12/29/14, 4:26 PM, Scott Feldman wrote:
>>> On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>>>> On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>>>>> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com>
>>>>> wrote:
>>>>>> On 12/29/14, 1:40 PM, Scott Feldman wrote:
>>>>>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>>>>>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>>>>>
>>>>>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to
>>>>>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows
>>>>>>>> userspace to pack more than one vlan in the setlink msg.
>>>>>>>>
>>>>>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>>>>> ---
>>>>>>>> net/bridge/br_netlink.c | 18 +++++++++---------
>>>>>>>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>>>>>>> index 9f5eb55..75971b1 100644
>>>>>>>> --- a/net/bridge/br_netlink.c
>>>>>>>> +++ b/net/bridge/br_netlink.c
>>>>>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br,
>>>>>>>> struct nlattr *af_spec,
>>>>>>>> int cmd)
>>>>>>>> {
>>>>>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1];
>>>>>>>> + struct bridge_vlan_info *vinfo;
>>>>>>>> int err = 0;
>>>>>>>> + struct nlattr *attr;
>>>>>>>> + int err = 0;
>>>>>>>> + int rem;
>>>>>>>> + u16 vid;
>>>>>>>>
>>>>>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec,
>>>>>>>> ifla_br_policy);
>>>>>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be
>>>>>>> removed?
>>>>>>
>>>>>> good question. Its a good place to see the type. In-fact userspace
>>>>>> programs
>>>>>> also copy the same policy to parse netlink attributes. hmmm..
>>>>>> I would like to keep it if it does not throw a warning.
>>>>> I don't know what the policy (sorry, no pun intended) on leaving dead
>>>>> code. I say remove it.
>>>> You know, not using the policy seems like a step backwards, and maybe
>>>> it suggests a problem with the attr packing.
>>>>
>>>> We had:
>>>>
>>>> ifla_br_policy
>>>> IFLA_BRIDGE_FLAGS
>>>> IFLA_BRIDGE_MODE
>>>> IFLA_BRIDGE_VLAN_INFO
>>>>
>>>> This patch set makes it:
>>>>
>>>> ifla_br_policy
>>>> IFLA_BRIDGE_FLAGS
>>>> IFLA_BRIDGE_MODE
>>>> IFLA_BRIDGE_VLAN_INFO
>>>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>>>
>>>> Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated.
>>>> I think you want some nesting to clarify:
>>>>
>>>> ifla_br_policy
>>>> IFLA_BRIDGE_FLAGS
>>>> IFLA_BRIDGE_MODE
>>>> IFLA_BRIDGE_VLAN_INFO
>>>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
>>>> IFLA_BRIDGE_VLAN_INFO
>>>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>>>
>>>> Now you can keep the policy for the top-level parsing, and loop only
>>>> on the nested array VLAN_LIST_INFO. Actually, now you can use just
>>>> RANGE_INFO in array and have:
>>>>
>>>> ifla_br_policy
>>>> IFLA_BRIDGE_FLAGS
>>>> IFLA_BRIDGE_MODE
>>>> IFLA_BRIDGE_VLAN_INFO
>>>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
>>>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>>>
>>>> And use VLAN_RANGE_INFO for both ranges of vids as well as single
>>>> vids. That'll simplify your filling algo in patch 5.
>>> Hmmmm...do you even need VLAN_RANGE_INFO? How about just using
>>> existing VLAN_INFO and add some more flags:
>>>
>>> #define BRIDGE_VLAN_INFO_RANGE_START (1<<3)
>>> #define BRIDGE_VLAN_INFO_RANGE_END (1<<4)
>>>
>>> Now you can have:
>>>
>>> IFLA_BRIDGE_FLAGS
>>> IFLA_BRIDGE_MODE
>>> IFLA_BRIDGE_VLAN_INFO
>>> IFLA_BRIDGE_VLAN_INFO_LIST // nested array of
>>> IFLA_BRIDGE_VLAN_INFO
>>>
>>> Don't set START or END for single vids in list.
>>
>> ok. I was debating yesterday about introducing another nest. This looks
>> good.
>> My only reason to not use existing IFLA_BRIDGE_VLAN_INFO was to make sure it
>> works for existing users.
>> I see that in this case since IFLA_BRIDGE_VLAN_INFO_LIST is new, it will not
>> affect existing users.
>>
>> But, i cant use IFLA_BRIDGE_VLAN_INFO (ie an attribute in ifla_br_policy)
>> under IFLA_BRIDGE_VLAN_INFO_LIST ?.
> I don't see why not. You're not going to parse the array with a
> policy anyway (since it's an array). And attr INFO_LIST will be .type
> = NLA_NESTED in ifla_br_policy.
>
>
agree that it will work if everybody assumes that this is an array of
just this attrtype.
wasn't sure if it is a good practice to reuse an attribute from an upper
nest.
will work on v2. thanks for the review.
^ permalink raw reply
* Re: [PATCH 6/6] bridge: add vlan info to bridge setlink and dellink notification messages
From: roopa @ 2014-12-30 5:41 UTC (permalink / raw)
To: Scott Feldman; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok
In-Reply-To: <CAE4R7bC406Vn_vCnbmoDNGMhNZTcO_sUu5CdOU7iefVOBZ-z6A@mail.gmail.com>
On 12/29/14, 9:25 PM, Scott Feldman wrote:
> On Mon, Dec 29, 2014 at 9:17 PM, roopa <roopa@cumulusnetworks.com> wrote:
>> On 12/29/14, 3:42 PM, Scott Feldman wrote:
>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>
>>>> vlan add/deletes are not notified to userspace today. This patch fixes
>>>> it.
>>>> Notifications will contain vlans compressed into ranges whereever
>>>> applicable.
>>> Why is this notification needed? On VLAN add/del, the port driver
>>> will already get called if it implements ndo_vlan_rx_add_vid and
>>> ndo_vlan_rx_kill_vid.
>> This is strictly for userspace.
>>
>>> The port driver already gets the notification
>>> it needs to filter vlans. User space can query for vlan port
>>> membership if it needs to know.
>>
>> You mean request a dump ?. yes it can if it needs all the configured vlans.
>> But for notifications, we must at the least notify what changed with respect
>> to vlans with RTM_SETLINK/DELLINK, no ?
> You'll get that notification if you implement ndo_vlan_rx_add/kill_vid
> in your port driver, which is called from RTM_SETLINK/DELLINK. Your
> port driver can send to user-space, I suppose, if user-space needs
> notification.
When the bridge is a vlan filtering bridge, I think its the bridge
driver who will need to send notifications to vlan add/dels
on PF_BRIDGE similar to fdb add/dels.
^ permalink raw reply
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC
From: Scott Feldman @ 2014-12-30 5:31 UTC (permalink / raw)
To: roopa; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok
In-Reply-To: <54A2374F.6050901@cumulusnetworks.com>
On Mon, Dec 29, 2014 at 9:25 PM, roopa <roopa@cumulusnetworks.com> wrote:
> On 12/29/14, 4:26 PM, Scott Feldman wrote:
>>
>> On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>>>
>>> On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>>>>
>>>> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com>
>>>> wrote:
>>>>>
>>>>> On 12/29/14, 1:40 PM, Scott Feldman wrote:
>>>>>>
>>>>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>>>>>>>
>>>>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>>>>
>>>>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to
>>>>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows
>>>>>>> userspace to pack more than one vlan in the setlink msg.
>>>>>>>
>>>>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>>>> ---
>>>>>>> net/bridge/br_netlink.c | 18 +++++++++---------
>>>>>>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>>>>>> index 9f5eb55..75971b1 100644
>>>>>>> --- a/net/bridge/br_netlink.c
>>>>>>> +++ b/net/bridge/br_netlink.c
>>>>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br,
>>>>>>> struct nlattr *af_spec,
>>>>>>> int cmd)
>>>>>>> {
>>>>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1];
>>>>>>> + struct bridge_vlan_info *vinfo;
>>>>>>> int err = 0;
>>>>>>> + struct nlattr *attr;
>>>>>>> + int err = 0;
>>>>>>> + int rem;
>>>>>>> + u16 vid;
>>>>>>>
>>>>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec,
>>>>>>> ifla_br_policy);
>>>>>>
>>>>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be
>>>>>> removed?
>>>>>
>>>>>
>>>>> good question. Its a good place to see the type. In-fact userspace
>>>>> programs
>>>>> also copy the same policy to parse netlink attributes. hmmm..
>>>>> I would like to keep it if it does not throw a warning.
>>>>
>>>> I don't know what the policy (sorry, no pun intended) on leaving dead
>>>> code. I say remove it.
>>>
>>> You know, not using the policy seems like a step backwards, and maybe
>>> it suggests a problem with the attr packing.
>>>
>>> We had:
>>>
>>> ifla_br_policy
>>> IFLA_BRIDGE_FLAGS
>>> IFLA_BRIDGE_MODE
>>> IFLA_BRIDGE_VLAN_INFO
>>>
>>> This patch set makes it:
>>>
>>> ifla_br_policy
>>> IFLA_BRIDGE_FLAGS
>>> IFLA_BRIDGE_MODE
>>> IFLA_BRIDGE_VLAN_INFO
>>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>>
>>> Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated.
>>> I think you want some nesting to clarify:
>>>
>>> ifla_br_policy
>>> IFLA_BRIDGE_FLAGS
>>> IFLA_BRIDGE_MODE
>>> IFLA_BRIDGE_VLAN_INFO
>>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
>>> IFLA_BRIDGE_VLAN_INFO
>>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>>
>>> Now you can keep the policy for the top-level parsing, and loop only
>>> on the nested array VLAN_LIST_INFO. Actually, now you can use just
>>> RANGE_INFO in array and have:
>>>
>>> ifla_br_policy
>>> IFLA_BRIDGE_FLAGS
>>> IFLA_BRIDGE_MODE
>>> IFLA_BRIDGE_VLAN_INFO
>>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
>>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>>
>>> And use VLAN_RANGE_INFO for both ranges of vids as well as single
>>> vids. That'll simplify your filling algo in patch 5.
>>
>> Hmmmm...do you even need VLAN_RANGE_INFO? How about just using
>> existing VLAN_INFO and add some more flags:
>>
>> #define BRIDGE_VLAN_INFO_RANGE_START (1<<3)
>> #define BRIDGE_VLAN_INFO_RANGE_END (1<<4)
>>
>> Now you can have:
>>
>> IFLA_BRIDGE_FLAGS
>> IFLA_BRIDGE_MODE
>> IFLA_BRIDGE_VLAN_INFO
>> IFLA_BRIDGE_VLAN_INFO_LIST // nested array of
>> IFLA_BRIDGE_VLAN_INFO
>>
>> Don't set START or END for single vids in list.
>
>
> ok. I was debating yesterday about introducing another nest. This looks
> good.
> My only reason to not use existing IFLA_BRIDGE_VLAN_INFO was to make sure it
> works for existing users.
> I see that in this case since IFLA_BRIDGE_VLAN_INFO_LIST is new, it will not
> affect existing users.
>
> But, i cant use IFLA_BRIDGE_VLAN_INFO (ie an attribute in ifla_br_policy)
> under IFLA_BRIDGE_VLAN_INFO_LIST ?.
I don't see why not. You're not going to parse the array with a
policy anyway (since it's an array). And attr INFO_LIST will be .type
= NLA_NESTED in ifla_br_policy.
>
> IFLA_BRIDGE_VLAN_INFO_LIST will have its own policy and its own attributes.
>
> Which will make it look something like below ?
>
> IFLA_BRIDGE_FLAGS
> IFLA_BRIDGE_MODE
> IFLA_BRIDGE_VLAN_INFO
> IFLA_BRIDGE_VLAN_INFO_LIST // nested array of
> IFLA_BRIDGE_VLAN_INFO_LIST_ENTRY
>
>
> Thanks,
> Roopa
^ permalink raw reply
* Re: [PATCH 5/6] bridge: new function to pack vlans using both IFLA_BRIDGE_VLAN_INFO and IFLA_BRIDGE_VLAN_RANGE_INFO
From: roopa @ 2014-12-30 5:31 UTC (permalink / raw)
To: Scott Feldman; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson kok
In-Reply-To: <CAE4R7bC63CpvpAuakFvEtEJvOsowjP40q1cs2eh0R=bf-F6w=Q@mail.gmail.com>
On 12/29/14, 3:25 PM, Scott Feldman wrote:
> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> This patch adds new function to compress vlans into ranges.
>> Vlans are compressed into ranges only if the fill request is called with
>> RTEXT_FILTER_BRVLAN_COMPRESSED in filtermask.
>>
>> Old vlan packing code is moved to a new function and continues to be
>> called when filter_mask is RTEXT_FILTER_BRVLAN
>>
>> Signed-off-by: Wilson kok <wkok@cumulusnetworks.com>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>> net/bridge/br_netlink.c | 157 +++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 137 insertions(+), 20 deletions(-)
>>
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 4c47ba0..16bdd5a 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -67,6 +67,133 @@ static int br_port_fill_attrs(struct sk_buff *skb,
>> return 0;
>> }
>>
>> +static int br_fill_ifvlaninfo_bitmap(struct sk_buff *skb,
>> + const unsigned long *vlan_bmp, u16 flags)
>> +{
>> + struct bridge_vlan_range_info vinfo_range;
>> + struct bridge_vlan_info vinfo;
>> + u16 vid, start = 0, end = 0;
> One per line?
ack
>> + u16 pvid;
> Aren't you getting an "unused" compile warning on this ^^^?
will double check..
>> +
>> + /* handle the untagged */
> This comment ^^^ doesn't make sense here.
>
>> + for_each_set_bit(vid, vlan_bmp, VLAN_N_VID) {
>> + if (start == 0) {
>> + start = vid;
>> + end = vid;
>> + }
>> + if ((vid - end) > 1) {
>> + memset(&vinfo_range, 0, sizeof(vinfo_range));
>> + vinfo_range.flags |= flags;
>> + vinfo_range.vid = start;
>> + vinfo_range.vid_end = end;
>> + if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO,
>> + sizeof(vinfo_range), &vinfo_range))
>> + goto nla_put_failure;
>> + start = vid;
>> + end = vid;
>> + } else {
>> + end = vid;
>> + }
>> + }
> What happens with this set {1-10, 12, 20-25, 30}? On vid 12, will
> that put a VLAN_RANGE_INFO with start=end=12? Seems strange to use
> RANGE_INFO for single vlan.
>
> Can the algorithm be simplified? Maybe there are other examples in
> the kernel of finding ranges/singles from a bitmap we could borrow
> code from?
let me see...
>> + if (start != 0 && end != 0) {
>> + if (start != end) {
>> + memset(&vinfo_range, 0, sizeof(vinfo_range));
>> + vinfo_range.flags |= flags;
>> + vinfo_range.vid = start;
>> + vinfo_range.vid_end = end;
>> + if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO,
>> + sizeof(vinfo_range), &vinfo_range))
>> + goto nla_put_failure;
>> + } else {
>> + memset(&vinfo, 0, sizeof(vinfo));
>> + vinfo.flags |= flags;
>> + vinfo.vid = start;
>> + if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
>> + sizeof(vinfo), &vinfo))
>> + goto nla_put_failure;
> How many IFLA_BRIDGE_VLAN_INFOs can you fit into skb? It seems the
> worst case is you'll have 4094/2 = 2047 VLAN_INFOs if using all
> even-numbered vids, for example. Will that fit? I guess the original
> code had the same concern...wonder if anyone checked this worst-case?
I will check this again. Remember doing worst case tests for setlinks.
will get back with some worst cases tests in the next submission.
>
>> + }
>> + }
>> +
>> +nla_put_failure:
>> + return -EMSGSIZE;
>> +}
>> +
>> +static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb,
>> + const struct net_port_vlans *pv)
>> +{
>> + unsigned long vlan_bmp_copy[BR_VLAN_BITMAP_LEN];
>> + unsigned long untagged_bmp_copy[BR_VLAN_BITMAP_LEN];
> Lots of automatic space on the stack...can you use dynamic mem
> (kzalloc) for these?
Let me see. Or make it a static global ?
>
>> + struct bridge_vlan_range_info vinfo_range;
>> + struct bridge_vlan_info vinfo;
>> + u16 pvid;
>> +
>> + memset(vlan_bmp_copy, 0,
>> + sizeof(unsigned long) * BR_VLAN_BITMAP_LEN);
>> + bitmap_copy(vlan_bmp_copy, pv->vlan_bitmap, VLAN_N_VID);
>> +
>> + memset(untagged_bmp_copy, 0,
>> + sizeof(unsigned long) * BR_VLAN_BITMAP_LEN);
>> + bitmap_copy(untagged_bmp_copy, pv->untagged_bitmap, VLAN_N_VID);
>> +
>> + /* send the pvid separately first */
>> + pvid = br_get_pvid(pv);
>> + if (pvid && (pvid != VLAN_N_VID)) {
>> + memset(&vinfo, 0, sizeof(vinfo));
>> + vinfo.flags |= BRIDGE_VLAN_INFO_PVID;
>> + if (test_bit(pvid, untagged_bmp_copy)) {
>> + vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>> + clear_bit(pvid, untagged_bmp_copy);
>> + }
>> + clear_bit(pvid, vlan_bmp_copy);
>> + vinfo.vid = pvid;
>> + if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
>> + sizeof(vinfo), &vinfo))
>> + goto nla_put_failure;
>> + }
> What if pvid is not in vlan_bmp_copy? This statement block seems
> slightly different than the original logic where BRIDGE_VLAN_INFO_PVID
> was set only when looping thru vlan_bitmap and vid == pvid. Now can
> you send a IFLA_BRIDGE_VLAN_INFO with pvid even if pvid isn't in
> vlan_bitmap?
yes, you can AFAIK. pvid is usually untagged.
> Maybe pvid is always in vlan_bitmap, so it doesn't
> matter. But there is a subtle logic change here, so something to
> consider.
Understand what you are saying, I will check this with v2.
>> +
>> + /* fill untagged vlans */
>> + br_fill_ifvlaninfo_bitmap(skb, untagged_bmp_copy,
>> + BRIDGE_VLAN_INFO_UNTAGGED);
> This might be doing more than original logic. Original logic looped
> thru vid in vlan_btimap and checked if vid was in untagged_bitmap.
> Here you're dumping all bits in untagged_bitmap, without looking if
> bit was set in vlan_bitmap. Maybe you want to do an intersection of
> sets vlan_bitmap and untagged_bitmap.
There is a clear_bit for vlan_bmp_copy below.
And all this works as expected on our boxes (Its tested code).
But, understand the difference with the original loop. Will confirm.
plus, we will see if we can use bitmap intersection here.
>> + for_each_set_bit(vid, untagged_bmp_copy, VLAN_N_VID)
>> + clear_bit(vid, vlan_bmp_copy);
>> +
>> + /* fill tagged vlans */
>> + br_fill_ifvlaninfo_bitmap(skb, vlan_bmp_copy, 0);
>> +
>> + return 0;
>> +
>> +nla_put_failure:
>> + return -EMSGSIZE;
>> +}
>> +
>> +static int br_fill_ifvlaninfo(struct sk_buff *skb,
>> + const struct net_port_vlans *pv)
>> +{
>> + struct bridge_vlan_info vinfo;
>> + u16 pvid, vid;
>> +
>> + pvid = br_get_pvid(pv);
>> + for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
>> + vinfo.vid = vid;
>> + vinfo.flags = 0;
>> + if (vid == pvid)
>> + vinfo.flags |= BRIDGE_VLAN_INFO_PVID;
>> +
>> + if (test_bit(vid, pv->untagged_bitmap))
>> + vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>> +
>> + if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
>> + sizeof(vinfo), &vinfo))
>> + goto nla_put_failure;
>> + }
>> +
>> + return 0;
>> +
>> +nla_put_failure:
>> + return -EMSGSIZE;
>> +}
>> +
>> /*
>> * Create one netlink message for one interface
>> * Contains port and master info as well as carrier and bridge state.
>> @@ -121,12 +248,11 @@ static int br_fill_ifinfo(struct sk_buff *skb,
>> }
>>
>> /* Check if the VID information is requested */
>> - if (filter_mask & RTEXT_FILTER_BRVLAN) {
>> - struct nlattr *af;
>> + if ((filter_mask & RTEXT_FILTER_BRVLAN) ||
>> + (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)) {
>> const struct net_port_vlans *pv;
>> - struct bridge_vlan_info vinfo;
>> - u16 vid;
>> - u16 pvid;
>> + struct nlattr *af;
>> + int err;
>>
>> if (port)
>> pv = nbp_get_vlan_info(port);
>> @@ -140,21 +266,12 @@ static int br_fill_ifinfo(struct sk_buff *skb,
>> if (!af)
>> goto nla_put_failure;
>>
>> - pvid = br_get_pvid(pv);
>> - for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
>> - vinfo.vid = vid;
>> - vinfo.flags = 0;
>> - if (vid == pvid)
>> - vinfo.flags |= BRIDGE_VLAN_INFO_PVID;
>> -
>> - if (test_bit(vid, pv->untagged_bitmap))
>> - vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>> -
>> - if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
>> - sizeof(vinfo), &vinfo))
>> - goto nla_put_failure;
>> - }
>> -
>> + if (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)
>> + err = br_fill_ifvlaninfo_compressed(skb, pv);
>> + else
>> + err = br_fill_ifvlaninfo(skb, pv);
>> + if (err)
>> + goto nla_put_failure;
>> nla_nest_end(skb, af);
>> }
>>
>> --
>> 1.7.10.4
>>
>> --
>> 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
^ permalink raw reply
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC
From: roopa @ 2014-12-30 5:25 UTC (permalink / raw)
To: Scott Feldman; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok
In-Reply-To: <CAE4R7bC7pgH35pkduV0n3G0UpoXoAUeJzmLE8ucJAwONJB2BZg@mail.gmail.com>
On 12/29/14, 4:26 PM, Scott Feldman wrote:
> On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>> On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>>> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com> wrote:
>>>> On 12/29/14, 1:40 PM, Scott Feldman wrote:
>>>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>>>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>>>
>>>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to
>>>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows
>>>>>> userspace to pack more than one vlan in the setlink msg.
>>>>>>
>>>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>>> ---
>>>>>> net/bridge/br_netlink.c | 18 +++++++++---------
>>>>>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>>>>> index 9f5eb55..75971b1 100644
>>>>>> --- a/net/bridge/br_netlink.c
>>>>>> +++ b/net/bridge/br_netlink.c
>>>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br,
>>>>>> struct nlattr *af_spec,
>>>>>> int cmd)
>>>>>> {
>>>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1];
>>>>>> + struct bridge_vlan_info *vinfo;
>>>>>> int err = 0;
>>>>>> + struct nlattr *attr;
>>>>>> + int err = 0;
>>>>>> + int rem;
>>>>>> + u16 vid;
>>>>>>
>>>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec,
>>>>>> ifla_br_policy);
>>>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be
>>>>> removed?
>>>>
>>>> good question. Its a good place to see the type. In-fact userspace programs
>>>> also copy the same policy to parse netlink attributes. hmmm..
>>>> I would like to keep it if it does not throw a warning.
>>> I don't know what the policy (sorry, no pun intended) on leaving dead
>>> code. I say remove it.
>> You know, not using the policy seems like a step backwards, and maybe
>> it suggests a problem with the attr packing.
>>
>> We had:
>>
>> ifla_br_policy
>> IFLA_BRIDGE_FLAGS
>> IFLA_BRIDGE_MODE
>> IFLA_BRIDGE_VLAN_INFO
>>
>> This patch set makes it:
>>
>> ifla_br_policy
>> IFLA_BRIDGE_FLAGS
>> IFLA_BRIDGE_MODE
>> IFLA_BRIDGE_VLAN_INFO
>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>
>> Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated.
>> I think you want some nesting to clarify:
>>
>> ifla_br_policy
>> IFLA_BRIDGE_FLAGS
>> IFLA_BRIDGE_MODE
>> IFLA_BRIDGE_VLAN_INFO
>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
>> IFLA_BRIDGE_VLAN_INFO
>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>
>> Now you can keep the policy for the top-level parsing, and loop only
>> on the nested array VLAN_LIST_INFO. Actually, now you can use just
>> RANGE_INFO in array and have:
>>
>> ifla_br_policy
>> IFLA_BRIDGE_FLAGS
>> IFLA_BRIDGE_MODE
>> IFLA_BRIDGE_VLAN_INFO
>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>
>> And use VLAN_RANGE_INFO for both ranges of vids as well as single
>> vids. That'll simplify your filling algo in patch 5.
> Hmmmm...do you even need VLAN_RANGE_INFO? How about just using
> existing VLAN_INFO and add some more flags:
>
> #define BRIDGE_VLAN_INFO_RANGE_START (1<<3)
> #define BRIDGE_VLAN_INFO_RANGE_END (1<<4)
>
> Now you can have:
>
> IFLA_BRIDGE_FLAGS
> IFLA_BRIDGE_MODE
> IFLA_BRIDGE_VLAN_INFO
> IFLA_BRIDGE_VLAN_INFO_LIST // nested array of
> IFLA_BRIDGE_VLAN_INFO
>
> Don't set START or END for single vids in list.
ok. I was debating yesterday about introducing another nest. This looks
good.
My only reason to not use existing IFLA_BRIDGE_VLAN_INFO was to make
sure it works for existing users.
I see that in this case since IFLA_BRIDGE_VLAN_INFO_LIST is new, it will
not affect existing users.
But, i cant use IFLA_BRIDGE_VLAN_INFO (ie an attribute in
ifla_br_policy) under IFLA_BRIDGE_VLAN_INFO_LIST ?.
IFLA_BRIDGE_VLAN_INFO_LIST will have its own policy and its own attributes.
Which will make it look something like below ?
IFLA_BRIDGE_FLAGS
IFLA_BRIDGE_MODE
IFLA_BRIDGE_VLAN_INFO
IFLA_BRIDGE_VLAN_INFO_LIST // nested array of
IFLA_BRIDGE_VLAN_INFO_LIST_ENTRY
Thanks,
Roopa
^ permalink raw reply
* Re: [PATCH 6/6] bridge: add vlan info to bridge setlink and dellink notification messages
From: Scott Feldman @ 2014-12-30 5:25 UTC (permalink / raw)
To: roopa; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok
In-Reply-To: <54A2355B.6060903@cumulusnetworks.com>
On Mon, Dec 29, 2014 at 9:17 PM, roopa <roopa@cumulusnetworks.com> wrote:
> On 12/29/14, 3:42 PM, Scott Feldman wrote:
>>
>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>>>
>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>>> vlan add/deletes are not notified to userspace today. This patch fixes
>>> it.
>>> Notifications will contain vlans compressed into ranges whereever
>>> applicable.
>>
>> Why is this notification needed? On VLAN add/del, the port driver
>> will already get called if it implements ndo_vlan_rx_add_vid and
>> ndo_vlan_rx_kill_vid.
>
> This is strictly for userspace.
>
>> The port driver already gets the notification
>> it needs to filter vlans. User space can query for vlan port
>> membership if it needs to know.
>
>
> You mean request a dump ?. yes it can if it needs all the configured vlans.
> But for notifications, we must at the least notify what changed with respect
> to vlans with RTM_SETLINK/DELLINK, no ?
You'll get that notification if you implement ndo_vlan_rx_add/kill_vid
in your port driver, which is called from RTM_SETLINK/DELLINK. Your
port driver can send to user-space, I suppose, if user-space needs
notification.
^ permalink raw reply
* Re: [PATCH 6/6] bridge: add vlan info to bridge setlink and dellink notification messages
From: roopa @ 2014-12-30 5:17 UTC (permalink / raw)
To: Scott Feldman; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok
In-Reply-To: <CAE4R7bCnqBX8TWmG4O_wppKtbnnCEWaaz+bHreybd8pPfUPs+w@mail.gmail.com>
On 12/29/14, 3:42 PM, Scott Feldman wrote:
> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> vlan add/deletes are not notified to userspace today. This patch fixes it.
>> Notifications will contain vlans compressed into ranges whereever applicable.
> Why is this notification needed? On VLAN add/del, the port driver
> will already get called if it implements ndo_vlan_rx_add_vid and
> ndo_vlan_rx_kill_vid.
This is strictly for userspace.
> The port driver already gets the notification
> it needs to filter vlans. User space can query for vlan port
> membership if it needs to know.
You mean request a dump ?. yes it can if it needs all the configured vlans.
But for notifications, we must at the least notify what changed with
respect to vlans with RTM_SETLINK/DELLINK, no ?
>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>> net/bridge/br_netlink.c | 3 ++-
>> net/core/rtnetlink.c | 3 ++-
>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 16bdd5a..cebfb03 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -303,7 +303,8 @@ void br_ifinfo_notify(int event, struct net_bridge_port *port)
>> if (skb == NULL)
>> goto errout;
>>
>> - err = br_fill_ifinfo(skb, port, 0, 0, event, 0, 0, port->dev);
>> + err = br_fill_ifinfo(skb, port, 0, 0, event, 0,
>> + RTEXT_FILTER_BRVLAN_COMPRESSED, port->dev);
> This doesn't look right. The skb wasn't allocated large enough to
> hold vlan info. If this is working in your tests, you're getting
> lucky due to some skb padding.
>
> skb was allocated with br_nlmsg_size(), which doesn't include vlan info.
>
>> if (err < 0) {
>> /* -EMSGSIZE implies BUG in br_nlmsg_size() */
>> WARN_ON(err == -EMSGSIZE);
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index d06107d..dad5fb6 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -2878,7 +2878,8 @@ static int rtnl_bridge_notify(struct net_device *dev, u16 flags)
>>
>> if ((!flags || (flags & BRIDGE_FLAGS_MASTER)) &&
>> br_dev && br_dev->netdev_ops->ndo_bridge_getlink) {
>> - err = br_dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev, 0);
>> + err = br_dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev,
>> + RTEXT_FILTER_BRVLAN_COMPRESSED);
>> if (err < 0)
>> goto errout;
>> }
>> --
>> 1.7.10.4
>>
>> --
>> 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
^ permalink raw reply
* Re: [PATCH] bonding: move ipoib_header_ops to vmlinux
From: David Miller @ 2014-12-30 4:25 UTC (permalink / raw)
To: wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA
Cc: cwang-xCSkyg8dI+0RB7SZvlqPiA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <54A21596.7000706-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
From: Wengang <wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Date: Tue, 30 Dec 2014 11:01:42 +0800
> There are more than one way we do things. For this case, considering
> needs, complexity and stability I think moving ipoib_header_ops is the
> right way to go.
I completely disagree, it's a gross hack at best.
It's papering over the real problem.
When we have references to released objects in other areas of the
networking stack, we don't move those objects into the static kernel
image as a fix.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] bonding: move ipoib_header_ops to vmlinux
From: David Miller @ 2014-12-30 4:23 UTC (permalink / raw)
To: wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1419908682-17012-1-git-send-email-wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
From: Wengang Wang <wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Date: Tue, 30 Dec 2014 11:04:42 +0800
> When last slave of a bonding master is removed, the bonding then does not work.
> At the time if packet_snd is called against with a master net_device, it calls
> then header_ops->create which points to slave's header_ops. In case the slave
> is ipoib and the module is unloaded, header_ops would point to invalid address.
> Accessing it will cause problem.
> This patch tries to fix this issue by moving ipoib_header_ops to vmlinux to keep
> it valid even when ipoib module is unloaded.
>
> Signed-off-by: Wengang Wang <wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Like others, I absolutely do not consider it sustainable to keep
moving header_ops implementations into the static kernel image.
We're just papering over the real problem, which is making sure all
dangling references to something really go away when we release/unload
an object.
Point it to a dummy set of ops and do a synchronize_net() or similar.
I'm not applying this patch, sorry.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] bonding: move ipoib_header_ops to vmlinux
From: Wengang Wang @ 2014-12-30 3:04 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
Cc: wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA
When last slave of a bonding master is removed, the bonding then does not work.
At the time if packet_snd is called against with a master net_device, it calls
then header_ops->create which points to slave's header_ops. In case the slave
is ipoib and the module is unloaded, header_ops would point to invalid address.
Accessing it will cause problem.
This patch tries to fix this issue by moving ipoib_header_ops to vmlinux to keep
it valid even when ipoib module is unloaded.
Signed-off-by: Wengang Wang <wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
drivers/infiniband/ulp/ipoib/ipoib.h | 10 ---------
drivers/infiniband/ulp/ipoib/ipoib_main.c | 28 +------------------------
include/linux/ibdevice.h | 15 ++++++++++++++
include/linux/if_infiniband.h | 11 ++++++++++
include/uapi/linux/if_infiniband.h | 16 ++++++++++++---
net/Makefile | 2 +-
net/infiniband/Makefile | 5 +++++
net/infiniband/infiniband.c | 34 +++++++++++++++++++++++++++++++
8 files changed, 80 insertions(+), 41 deletions(-)
create mode 100644 include/linux/ibdevice.h
create mode 100644 include/linux/if_infiniband.h
create mode 100644 net/infiniband/Makefile
create mode 100644 net/infiniband/infiniband.c
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index d7562be..7c25670 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -121,16 +121,6 @@ enum {
/* structs */
-struct ipoib_header {
- __be16 proto;
- u16 reserved;
-};
-
-struct ipoib_cb {
- struct qdisc_skb_cb qdisc_cb;
- u8 hwaddr[INFINIBAND_ALEN];
-};
-
static inline struct ipoib_cb *ipoib_skb_cb(const struct sk_buff *skb)
{
BUILD_BUG_ON(sizeof(skb->cb) < sizeof(struct ipoib_cb));
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 58b5aa3..9233085 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -34,6 +34,7 @@
#include "ipoib.h"
+#include <linux/ibdevice.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -807,29 +808,6 @@ static void ipoib_timeout(struct net_device *dev)
/* XXX reset QP, etc. */
}
-static int ipoib_hard_header(struct sk_buff *skb,
- struct net_device *dev,
- unsigned short type,
- const void *daddr, const void *saddr, unsigned len)
-{
- struct ipoib_header *header;
- struct ipoib_cb *cb = ipoib_skb_cb(skb);
-
- header = (struct ipoib_header *) skb_push(skb, sizeof *header);
-
- header->proto = htons(type);
- header->reserved = 0;
-
- /*
- * we don't rely on dst_entry structure, always stuff the
- * destination address into skb->cb so we can figure out where
- * to send the packet later.
- */
- memcpy(cb->hwaddr, daddr, INFINIBAND_ALEN);
-
- return sizeof *header;
-}
-
static void ipoib_set_mcast_list(struct net_device *dev)
{
struct ipoib_dev_priv *priv = netdev_priv(dev);
@@ -1328,10 +1306,6 @@ void ipoib_dev_cleanup(struct net_device *dev)
ipoib_neigh_hash_uninit(dev);
}
-static const struct header_ops ipoib_header_ops = {
- .create = ipoib_hard_header,
-};
-
static const struct net_device_ops ipoib_netdev_ops = {
.ndo_uninit = ipoib_uninit,
.ndo_open = ipoib_open,
diff --git a/include/linux/ibdevice.h b/include/linux/ibdevice.h
new file mode 100644
index 0000000..8418974
--- /dev/null
+++ b/include/linux/ibdevice.h
@@ -0,0 +1,15 @@
+/*
+ * ipoib Implementation of ipoib_header_ops here.
+ *
+ * Authors: Wengang Wang <wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
+ */
+#ifndef _LINUX_IBDEVICE_H
+#define _LINUX_IBDEVICE_H
+
+#include <linux/netdevice.h>
+
+#ifdef __KERNEL__
+extern const struct header_ops ipoib_header_ops;
+#endif /* __KERNEL__ */
+
+#endif /* _LINUX_IBDEVICE_H */
diff --git a/include/linux/if_infiniband.h b/include/linux/if_infiniband.h
new file mode 100644
index 0000000..9f2d0cf
--- /dev/null
+++ b/include/linux/if_infiniband.h
@@ -0,0 +1,11 @@
+/*
+ * ipoib Implementation of ipoib_header_ops here.
+ *
+ * Authors: Wengang Wang <wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
+ */
+#ifndef _LINUX_IF_INFINIBAND_H
+#define _LINUX_IF_INFINIBAND_H
+
+#include <uapi/linux/if_infiniband.h>
+
+#endif /* _LINUX_IF_INFINIBAND_H */
diff --git a/include/uapi/linux/if_infiniband.h b/include/uapi/linux/if_infiniband.h
index 7d958475..9190ee3 100644
--- a/include/uapi/linux/if_infiniband.h
+++ b/include/uapi/linux/if_infiniband.h
@@ -21,9 +21,19 @@
* $Id$
*/
-#ifndef _LINUX_IF_INFINIBAND_H
-#define _LINUX_IF_INFINIBAND_H
+#ifndef _UAPI_LINUX_IF_INFINIBAND_H
+#define _UAPI_LINUX_IF_INFINIBAND_H
+#include <net/sch_generic.h>
#define INFINIBAND_ALEN 20 /* Octets in IPoIB HW addr */
-#endif /* _LINUX_IF_INFINIBAND_H */
+struct ipoib_header {
+ __be16 proto;
+ u16 reserved;
+};
+
+struct ipoib_cb {
+ struct qdisc_skb_cb qdisc_cb;
+ u8 hwaddr[INFINIBAND_ALEN];
+};
+#endif /* _UAPI_LINUX_IF_INFINIBAND_H */
diff --git a/net/Makefile b/net/Makefile
index 7ed1970..5d00a13 100644
--- a/net/Makefile
+++ b/net/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_NET) += $(tmp-y)
# LLC has to be linked before the files in net/802/
obj-$(CONFIG_LLC) += llc/
-obj-$(CONFIG_NET) += ethernet/ 802/ sched/ netlink/
+obj-$(CONFIG_NET) += ethernet/ 802/ sched/ netlink/ infiniband/
obj-$(CONFIG_NETFILTER) += netfilter/
obj-$(CONFIG_INET) += ipv4/
obj-$(CONFIG_XFRM) += xfrm/
diff --git a/net/infiniband/Makefile b/net/infiniband/Makefile
new file mode 100644
index 0000000..c8a5be0
--- /dev/null
+++ b/net/infiniband/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for the Linux ip over infiniband layer.
+#
+
+obj-y += infiniband.o
diff --git a/net/infiniband/infiniband.c b/net/infiniband/infiniband.c
new file mode 100644
index 0000000..a458ec5
--- /dev/null
+++ b/net/infiniband/infiniband.c
@@ -0,0 +1,34 @@
+/*
+ * ipoib Implementation of ipoib_header_ops here.
+ *
+ * Authors: Wengang Wang <wen.gang.wang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
+ */
+
+#include <linux/if_infiniband.h>
+
+static int ipoib_hard_header(struct sk_buff *skb,
+ struct net_device *dev,
+ unsigned short type,
+ const void *daddr, const void *saddr, unsigned len)
+{
+ struct ipoib_header *header;
+ struct ipoib_cb *cb = (struct ipoib_cb *)skb->cb;
+
+ header = (struct ipoib_header *)skb_push(skb, sizeof(*header));
+
+ header->proto = htons(type);
+ header->reserved = 0;
+
+ /* we don't rely on dst_entry structure, always stuff the
+ * destination address into skb->cb so we can figure out where
+ * to send the packet later.
+ */
+ memcpy(cb->hwaddr, daddr, INFINIBAND_ALEN);
+
+ return sizeof(*header);
+}
+
+const struct header_ops ipoib_header_ops = {
+ .create = ipoib_hard_header,
+};
+EXPORT_SYMBOL(ipoib_header_ops);
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH] bonding: move ipoib_header_ops to vmlinux
From: Wengang @ 2014-12-30 3:01 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAHA+R7M-Nm_R-AaKQ0nX4L3O=BN5_m1v-8sWJSaE_UmGyo8zwA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
于 2014年12月30日 05:32, Cong Wang 写道:
> On Mon, Nov 24, 2014 at 9:36 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
>> When last slave of a bonding master is removed, the bonding then does not work.
>> At the time if packet_snd is called against with a master net_device, it calls
>> then header_ops->create which points to slave's header_ops. In case the slave
>> is ipoib and the module is unloaded, header_ops would point to invalid address.
>> Accessing it will cause problem.
>> This patch tries to fix this issue by moving ipoib_header_ops to vmlinux to keep
>> it valid even when ipoib module is unloaded.
>>
> I still don't think this is the right way to fix the bug, because
> there are other modules which have the same bug, for example,
> net/bluetooth/6lowpan.c (unless it can't be enslaved into a bond
> of course, I haven't verified). We don't want to move them all
> to builtin kernel, do we?
I don't think bluetooth needs bonding(and you didn't verify at all).
Do you have strong reason(examples)?
> On the other hand, as Jay explained, bonding is probably
> the only one which has the problem, why not solve it in bonding?
> I mean why do we have to adjust other modules just for bonding?
There are more than one way we do things. For this case, considering
needs, complexity and stability I think moving ipoib_header_ops is the
right way to go.
> When its slave device gets removed, some netdev event is sent
> to the master, so why not reset the header_ops to ether header ops
> up on this event? Something like below (not even compile tested):
>
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 184c434..aedccae 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1726,6 +1726,7 @@ static int __bond_release_one(struct net_device *bond_dev,
> if (!bond_has_slaves(bond)) {
> bond_set_carrier(bond);
> eth_hw_addr_random(bond_dev);
> + ether_setup(bond_dev);
> }
>
> unblock_netpoll_tx();
>
> It should solve more than just your ipoib case.
Do you think it's safe in a race condition? Before you change
header_ops, you have no
way to prevent another thread from accessing the old one.
> Last but not least, since you said you saw the crash in packet_snd(),
> it would also be interesting to know why packet_snd() still picks this
> bond dev after all its slaves are gone, after all it is not a functional
> device without any slave (its carrier is off).
It's quite normal in a race situation.
thanks,
wengang
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC
From: Scott Feldman @ 2014-12-30 0:26 UTC (permalink / raw)
To: roopa; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok
In-Reply-To: <CAE4R7bAHSNePcX3aAa72kYReC4c8fRKF+BzL1f-bdk-M_pLxUg@mail.gmail.com>
On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman <sfeldma@gmail.com> wrote:
> On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com> wrote:
>>> On 12/29/14, 1:40 PM, Scott Feldman wrote:
>>>>
>>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>>>>>
>>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>>
>>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to
>>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows
>>>>> userspace to pack more than one vlan in the setlink msg.
>>>>>
>>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>> ---
>>>>> net/bridge/br_netlink.c | 18 +++++++++---------
>>>>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>>>> index 9f5eb55..75971b1 100644
>>>>> --- a/net/bridge/br_netlink.c
>>>>> +++ b/net/bridge/br_netlink.c
>>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br,
>>>>> struct nlattr *af_spec,
>>>>> int cmd)
>>>>> {
>>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1];
>>>>> + struct bridge_vlan_info *vinfo;
>>>>> int err = 0;
>>>>> + struct nlattr *attr;
>>>>> + int err = 0;
>>>>> + int rem;
>>>>> + u16 vid;
>>>>>
>>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec,
>>>>> ifla_br_policy);
>>>>
>>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be
>>>> removed?
>>>
>>>
>>> good question. Its a good place to see the type. In-fact userspace programs
>>> also copy the same policy to parse netlink attributes. hmmm..
>>> I would like to keep it if it does not throw a warning.
>>
>> I don't know what the policy (sorry, no pun intended) on leaving dead
>> code. I say remove it.
>
> You know, not using the policy seems like a step backwards, and maybe
> it suggests a problem with the attr packing.
>
> We had:
>
> ifla_br_policy
> IFLA_BRIDGE_FLAGS
> IFLA_BRIDGE_MODE
> IFLA_BRIDGE_VLAN_INFO
>
> This patch set makes it:
>
> ifla_br_policy
> IFLA_BRIDGE_FLAGS
> IFLA_BRIDGE_MODE
> IFLA_BRIDGE_VLAN_INFO
> IFLA_BRIDGE_VLAN_RANGE_INFO
>
> Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated.
> I think you want some nesting to clarify:
>
> ifla_br_policy
> IFLA_BRIDGE_FLAGS
> IFLA_BRIDGE_MODE
> IFLA_BRIDGE_VLAN_INFO
> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
> IFLA_BRIDGE_VLAN_INFO
> IFLA_BRIDGE_VLAN_RANGE_INFO
>
> Now you can keep the policy for the top-level parsing, and loop only
> on the nested array VLAN_LIST_INFO. Actually, now you can use just
> RANGE_INFO in array and have:
>
> ifla_br_policy
> IFLA_BRIDGE_FLAGS
> IFLA_BRIDGE_MODE
> IFLA_BRIDGE_VLAN_INFO
> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
> IFLA_BRIDGE_VLAN_RANGE_INFO
>
> And use VLAN_RANGE_INFO for both ranges of vids as well as single
> vids. That'll simplify your filling algo in patch 5.
Hmmmm...do you even need VLAN_RANGE_INFO? How about just using
existing VLAN_INFO and add some more flags:
#define BRIDGE_VLAN_INFO_RANGE_START (1<<3)
#define BRIDGE_VLAN_INFO_RANGE_END (1<<4)
Now you can have:
IFLA_BRIDGE_FLAGS
IFLA_BRIDGE_MODE
IFLA_BRIDGE_VLAN_INFO
IFLA_BRIDGE_VLAN_INFO_LIST // nested array of
IFLA_BRIDGE_VLAN_INFO
Don't set START or END for single vids in list.
^ permalink raw reply
* Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC
From: Scott Feldman @ 2014-12-30 0:07 UTC (permalink / raw)
To: roopa; +Cc: Netdev, shemminger, vyasevic@redhat.com, Wilson Kok
In-Reply-To: <CAE4R7bDiRd6WY7ry-vFCj2HgAFtxWznwJTX4vqxt_v5Q8LXdOg@mail.gmail.com>
On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@gmail.com> wrote:
> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@cumulusnetworks.com> wrote:
>> On 12/29/14, 1:40 PM, Scott Feldman wrote:
>>>
>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@cumulusnetworks.com> wrote:
>>>>
>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>
>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to
>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows
>>>> userspace to pack more than one vlan in the setlink msg.
>>>>
>>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>> ---
>>>> net/bridge/br_netlink.c | 18 +++++++++---------
>>>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>>> index 9f5eb55..75971b1 100644
>>>> --- a/net/bridge/br_netlink.c
>>>> +++ b/net/bridge/br_netlink.c
>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br,
>>>> struct nlattr *af_spec,
>>>> int cmd)
>>>> {
>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1];
>>>> + struct bridge_vlan_info *vinfo;
>>>> int err = 0;
>>>> + struct nlattr *attr;
>>>> + int err = 0;
>>>> + int rem;
>>>> + u16 vid;
>>>>
>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec,
>>>> ifla_br_policy);
>>>
>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be
>>> removed?
>>
>>
>> good question. Its a good place to see the type. In-fact userspace programs
>> also copy the same policy to parse netlink attributes. hmmm..
>> I would like to keep it if it does not throw a warning.
>
> I don't know what the policy (sorry, no pun intended) on leaving dead
> code. I say remove it.
You know, not using the policy seems like a step backwards, and maybe
it suggests a problem with the attr packing.
We had:
ifla_br_policy
IFLA_BRIDGE_FLAGS
IFLA_BRIDGE_MODE
IFLA_BRIDGE_VLAN_INFO
This patch set makes it:
ifla_br_policy
IFLA_BRIDGE_FLAGS
IFLA_BRIDGE_MODE
IFLA_BRIDGE_VLAN_INFO
IFLA_BRIDGE_VLAN_RANGE_INFO
Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated.
I think you want some nesting to clarify:
ifla_br_policy
IFLA_BRIDGE_FLAGS
IFLA_BRIDGE_MODE
IFLA_BRIDGE_VLAN_INFO
IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
IFLA_BRIDGE_VLAN_INFO
IFLA_BRIDGE_VLAN_RANGE_INFO
Now you can keep the policy for the top-level parsing, and loop only
on the nested array VLAN_LIST_INFO. Actually, now you can use just
RANGE_INFO in array and have:
ifla_br_policy
IFLA_BRIDGE_FLAGS
IFLA_BRIDGE_MODE
IFLA_BRIDGE_VLAN_INFO
IFLA_BRIDGE_VLAN_LIST_INFO // nested array of
IFLA_BRIDGE_VLAN_RANGE_INFO
And use VLAN_RANGE_INFO for both ranges of vids as well as single
vids. That'll simplify your filling algo in patch 5.
-scott
^ permalink raw reply
* [PATCH iproute2 2/2] ss: Unify packet stats output from netlink and proc
From: Vadim Kochan @ 2014-12-29 23:56 UTC (permalink / raw)
To: netdev; +Cc: Vadim Kochan
In-Reply-To: <1419897380-21012-1-git-send-email-vadim4j@gmail.com>
From: Vadim Kochan <vadim4j@gmail.com>
Refactored to use one func for output packet stats info
from both /proc and netlink.
Added possibility to get packet stats info from /proc
by setting environment variable PROC_ROOT or PROC_NET_PACKET.
Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
misc/ss.c | 211 ++++++++++++++++++++++++++++----------------------------------
1 file changed, 95 insertions(+), 116 deletions(-)
diff --git a/misc/ss.c b/misc/ss.c
index 88b14f8..641c56b 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2536,69 +2536,86 @@ static int unix_show(struct filter *f)
return 0;
}
-static int packet_show_sock(const struct sockaddr_nl *addr,
- struct nlmsghdr *nlh, void *arg)
-{
- struct packet_diag_msg *r = NLMSG_DATA(nlh);
- struct rtattr *tb[PACKET_DIAG_MAX+1];
- __u32 rq;
-
- parse_rtattr(tb, PACKET_DIAG_MAX, (struct rtattr*)(r+1),
- nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
+struct pktstat {
+ int type;
+ int prot;
+ int iface;
+ int state;
+ int rq;
+ int uid;
+ int ino;
+};
- /* use /proc/net/packet if all info are not available */
- if (!tb[PACKET_DIAG_MEMINFO])
- return -1;
+static void packet_stats_print(struct pktstat *s)
+{
+ char *buf = NULL;
if (netid_width)
printf("%-*s ", netid_width,
- r->pdiag_type == SOCK_RAW ? "p_raw" : "p_dgr");
+ s->type == SOCK_RAW ? "p_raw" : "p_dgr");
if (state_width)
printf("%-*s ", state_width, "UNCONN");
- if (tb[PACKET_DIAG_MEMINFO]) {
- __u32 *skmeminfo = RTA_DATA(tb[PACKET_DIAG_MEMINFO]);
-
- rq = skmeminfo[SK_MEMINFO_RMEM_ALLOC];
- } else
- rq = 0;
- printf("%-6d %-6d ", rq, 0);
-
- if (r->pdiag_num == 3) {
+ printf("%-6d %-6d ", s->rq, 0);
+ if (s->prot == 3) {
printf("%*s:", addr_width, "*");
} else {
- char tb2[16];
+ char tb[16];
printf("%*s:", addr_width,
- ll_proto_n2a(htons(r->pdiag_num), tb2, sizeof(tb2)));
+ ll_proto_n2a(htons(s->prot), tb, sizeof(tb)));
}
- if (tb[PACKET_DIAG_INFO]) {
- struct packet_diag_info *pinfo = RTA_DATA(tb[PACKET_DIAG_INFO]);
-
- if (pinfo->pdi_index == 0)
- printf("%-*s ", serv_width, "*");
- else
- printf("%-*s ", serv_width, xll_index_to_name(pinfo->pdi_index));
- } else
+ if (s->iface == 0) {
printf("%-*s ", serv_width, "*");
+ } else {
+ printf("%-*s ", serv_width, xll_index_to_name(s->iface));
+ }
- printf("%*s*%-*s",
- addr_width, "", serv_width, "");
-
- char *buf = NULL;
+ printf("%*s*%-*s", addr_width, "", serv_width, "");
if (show_proc_ctx || show_sock_ctx) {
- if (find_entry(r->pdiag_ino, &buf,
- (show_proc_ctx & show_sock_ctx) ?
- PROC_SOCK_CTX : PROC_CTX) > 0) {
+ if (find_entry(s->ino, &buf,
+ (show_proc_ctx & show_sock_ctx) ?
+ PROC_SOCK_CTX : PROC_CTX) > 0) {
printf(" users:(%s)", buf);
free(buf);
}
} else if (show_users) {
- if (find_entry(r->pdiag_ino, &buf, USERS) > 0) {
+ if (find_entry(s->ino, &buf, USERS) > 0) {
printf(" users:(%s)", buf);
free(buf);
}
}
+}
+
+static int packet_show_sock(const struct sockaddr_nl *addr,
+ struct nlmsghdr *nlh, void *arg)
+{
+ struct packet_diag_msg *r = NLMSG_DATA(nlh);
+ struct rtattr *tb[PACKET_DIAG_MAX+1];
+ struct pktstat stat = {};
+
+ parse_rtattr(tb, PACKET_DIAG_MAX, (struct rtattr*)(r+1),
+ nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
+
+ /* use /proc/net/packet if all info are not available */
+ if (!tb[PACKET_DIAG_MEMINFO])
+ return -1;
+
+ stat.type = r->pdiag_type;
+ stat.prot = r->pdiag_num;
+ stat.ino = r->pdiag_ino;
+
+ if (tb[PACKET_DIAG_MEMINFO]) {
+ __u32 *skmeminfo = RTA_DATA(tb[PACKET_DIAG_MEMINFO]);
+ stat.rq = skmeminfo[SK_MEMINFO_RMEM_ALLOC];
+ }
+
+ if (tb[PACKET_DIAG_INFO]) {
+ struct packet_diag_info *pinfo = RTA_DATA(tb[PACKET_DIAG_INFO]);
+ stat.iface = pinfo->pdi_index;
+ }
+
+ packet_stats_print(&stat);
if (show_details) {
__u32 uid = 0;
@@ -2640,94 +2657,56 @@ static int packet_show_netlink(struct filter *f)
return handle_netlink_request(f, &req.nlh, sizeof(req), packet_show_sock);
}
+static int packet_show_line(char *buf, const struct filter *f, int fam)
+{
+ unsigned long long sk;
+ struct pktstat stat = {};
+
+ sscanf(buf, "%llx %*d %d %x %d %d %u %u %u",
+ &sk,
+ &stat.type, &stat.prot, &stat.iface, &stat.state,
+ &stat.rq, &stat.uid, &stat.ino);
+
+ if (stat.type == SOCK_RAW && !(f->dbs&(1<<PACKET_R_DB)))
+ return 0;
+ if (stat.type == SOCK_DGRAM && !(f->dbs&(1<<PACKET_DG_DB)))
+ return 0;
+ if (f->f) {
+ struct tcpstat tst;
+ tst.local.family = AF_PACKET;
+ tst.remote.family = AF_PACKET;
+ tst.rport = 0;
+ tst.lport = stat.iface;
+ tst.local.data[0] = stat.prot;
+ tst.remote.data[0] = 0;
+ if (run_ssfilter(f->f, &tst) == 0)
+ return 0;
+ }
+
+ packet_stats_print(&stat);
+
+ if (show_details) {
+ printf(" ino=%u uid=%u sk=%llx", stat.ino, stat.uid, sk);
+ }
+ printf("\n");
+ return 0;
+}
static int packet_show(struct filter *f)
{
FILE *fp;
- char buf[256];
- int type;
- int prot;
- int iface;
- int state;
- int rq;
- int uid;
- int ino;
- unsigned long long sk;
if (preferred_family != AF_PACKET && !(f->states & (1 << SS_CLOSE)))
return 0;
- if (packet_show_netlink(f) == 0)
+ if (!getenv("PROC_NET_PACKET") && !getenv("PROC_ROOT") &&
+ packet_show_netlink(f) == 0)
return 0;
if ((fp = net_packet_open()) == NULL)
return -1;
- fgets(buf, sizeof(buf)-1, fp);
-
- while (fgets(buf, sizeof(buf)-1, fp)) {
- sscanf(buf, "%llx %*d %d %x %d %d %u %u %u",
- &sk,
- &type, &prot, &iface, &state,
- &rq, &uid, &ino);
-
- if (type == SOCK_RAW && !(f->dbs&(1<<PACKET_R_DB)))
- continue;
- if (type == SOCK_DGRAM && !(f->dbs&(1<<PACKET_DG_DB)))
- continue;
- if (f->f) {
- struct tcpstat tst;
- tst.local.family = AF_PACKET;
- tst.remote.family = AF_PACKET;
- tst.rport = 0;
- tst.lport = iface;
- tst.local.data[0] = prot;
- tst.remote.data[0] = 0;
- if (run_ssfilter(f->f, &tst) == 0)
- continue;
- }
-
- if (netid_width)
- printf("%-*s ", netid_width,
- type == SOCK_RAW ? "p_raw" : "p_dgr");
- if (state_width)
- printf("%-*s ", state_width, "UNCONN");
- printf("%-6d %-6d ", rq, 0);
- if (prot == 3) {
- printf("%*s:", addr_width, "*");
- } else {
- char tb[16];
- printf("%*s:", addr_width,
- ll_proto_n2a(htons(prot), tb, sizeof(tb)));
- }
- if (iface == 0) {
- printf("%-*s ", serv_width, "*");
- } else {
- printf("%-*s ", serv_width, xll_index_to_name(iface));
- }
- printf("%*s*%-*s",
- addr_width, "", serv_width, "");
-
- char *buf = NULL;
-
- if (show_proc_ctx || show_sock_ctx) {
- if (find_entry(ino, &buf,
- (show_proc_ctx & show_sock_ctx) ?
- PROC_SOCK_CTX : PROC_CTX) > 0) {
- printf(" users:(%s)", buf);
- free(buf);
- }
- } else if (show_users) {
- if (find_entry(ino, &buf, USERS) > 0) {
- printf(" users:(%s)", buf);
- free(buf);
- }
- }
-
- if (show_details) {
- printf(" ino=%u uid=%u sk=%llx", ino, uid, sk);
- }
- printf("\n");
- }
+ if (generic_record_read(fp, packet_show_line, f, AF_PACKET))
+ return -1;
return 0;
}
--
2.1.3
^ permalink raw reply related
* [PATCH iproute2 1/2] ss: Unify unix stats output from netlink and proc
From: Vadim Kochan @ 2014-12-29 23:56 UTC (permalink / raw)
To: netdev; +Cc: Vadim Kochan
In-Reply-To: <1419897380-21012-1-git-send-email-vadim4j@gmail.com>
From: Vadim Kochan <vadim4j@gmail.com>
Refactored to use one func for output unix stats info
from both /proc and netlink.
Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
misc/ss.c | 99 ++++++++++++++++++++++++---------------------------------------
1 file changed, 37 insertions(+), 62 deletions(-)
diff --git a/misc/ss.c b/misc/ss.c
index f0c7b34..88b14f8 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2234,6 +2234,7 @@ struct unixstat
struct unixstat *next;
int ino;
int peer;
+ char *peer_name;
int rq;
int wq;
int state;
@@ -2279,7 +2280,18 @@ static const char *unix_netid_name(int type)
return netid;
}
-static void unix_list_print(struct unixstat *list, struct filter *f)
+static int unix_type_skip(struct unixstat *s, struct filter *f)
+{
+ if (s->type == SOCK_STREAM && !(f->dbs&(1<<UNIX_ST_DB)))
+ return 1;
+ if (s->type == SOCK_DGRAM && !(f->dbs&(1<<UNIX_DG_DB)))
+ return 1;
+ if (s->type == SOCK_SEQPACKET && !(f->dbs&(1<<UNIX_SQ_DB)))
+ return 1;
+ return 0;
+}
+
+static void unix_stats_print(struct unixstat *list, struct filter *f)
{
struct unixstat *s;
char *peer;
@@ -2287,15 +2299,14 @@ static void unix_list_print(struct unixstat *list, struct filter *f)
for (s = list; s; s = s->next) {
if (!(f->states & (1<<s->state)))
continue;
- if (s->type == SOCK_STREAM && !(f->dbs&(1<<UNIX_ST_DB)))
- continue;
- if (s->type == SOCK_DGRAM && !(f->dbs&(1<<UNIX_DG_DB)))
- continue;
- if (s->type == SOCK_SEQPACKET && !(f->dbs&(1<<UNIX_SQ_DB)))
+ if (unix_type_skip(s, f))
continue;
peer = "*";
- if (s->peer) {
+ if (s->peer_name)
+ peer = s->peer_name;
+
+ if (s->peer && !s->peer_name) {
struct unixstat *p;
for (p = list; p; p = p->next) {
if (s->peer == p->ino)
@@ -2356,36 +2367,24 @@ static int unix_show_sock(const struct sockaddr_nl *addr, struct nlmsghdr *nlh,
struct unix_diag_msg *r = NLMSG_DATA(nlh);
struct rtattr *tb[UNIX_DIAG_MAX+1];
char name[128];
- int peer_ino;
- __u32 rqlen, wqlen;
+ struct unixstat stat = { .name = "*" , .peer_name = "*" };
parse_rtattr(tb, UNIX_DIAG_MAX, (struct rtattr*)(r+1),
nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
- if (r->udiag_type == SOCK_STREAM && !(f->dbs&(1<<UNIX_ST_DB)))
- return 0;
- if (r->udiag_type == SOCK_DGRAM && !(f->dbs&(1<<UNIX_DG_DB)))
- return 0;
- if (r->udiag_type == SOCK_SEQPACKET && !(f->dbs&(1<<UNIX_SQ_DB)))
- return 0;
+ f->f = NULL;
+ stat.type = r->udiag_type;
+ stat.state = r->udiag_state;
+ stat.ino = r->udiag_ino;
- if (netid_width)
- printf("%-*s ", netid_width,
- unix_netid_name(r->udiag_type));
- if (state_width)
- printf("%-*s ", state_width, sstate_name[r->udiag_state]);
+ if (unix_type_skip(&stat, f))
+ return 0;
if (tb[UNIX_DIAG_RQLEN]) {
struct unix_diag_rqlen *rql = RTA_DATA(tb[UNIX_DIAG_RQLEN]);
- rqlen = rql->udiag_rqueue;
- wqlen = rql->udiag_wqueue;
- } else {
- rqlen = 0;
- wqlen = 0;
+ stat.rq = rql->udiag_rqueue;
+ stat.wq = rql->udiag_wqueue;
}
-
- printf("%-6u %-6u ", rqlen, wqlen);
-
if (tb[UNIX_DIAG_NAME]) {
int len = RTA_PAYLOAD(tb[UNIX_DIAG_NAME]);
@@ -2393,41 +2392,17 @@ static int unix_show_sock(const struct sockaddr_nl *addr, struct nlmsghdr *nlh,
name[len] = '\0';
if (name[0] == '\0')
name[0] = '@';
- } else
- sprintf(name, "*");
-
+ stat.name = &name[0];
+ }
if (tb[UNIX_DIAG_PEER])
- peer_ino = rta_getattr_u32(tb[UNIX_DIAG_PEER]);
- else
- peer_ino = 0;
-
- printf("%*s %-*d %*s %-*d",
- addr_width, name,
- serv_width, r->udiag_ino,
- addr_width, "*", /* FIXME */
- serv_width, peer_ino);
-
- char *buf = NULL;
+ stat.peer = rta_getattr_u32(tb[UNIX_DIAG_PEER]);
- if (show_proc_ctx || show_sock_ctx) {
- if (find_entry(r->udiag_ino, &buf,
- (show_proc_ctx & show_sock_ctx) ?
- PROC_SOCK_CTX : PROC_CTX) > 0) {
- printf(" users:(%s)", buf);
- free(buf);
- }
- } else if (show_users) {
- if (find_entry(r->udiag_ino, &buf, USERS) > 0) {
- printf(" users:(%s)", buf);
- free(buf);
- }
- }
+ unix_stats_print(&stat, f);
if (show_mem) {
- printf("\n\t");
+ printf("\t");
print_skmeminfo(tb, UNIX_DIAG_MEMINFO);
}
-
if (show_details) {
if (tb[UNIX_DIAG_SHUTDOWN]) {
unsigned char mask;
@@ -2435,9 +2410,8 @@ static int unix_show_sock(const struct sockaddr_nl *addr, struct nlmsghdr *nlh,
printf(" %c-%c", mask & 1 ? '-' : '<', mask & 2 ? '-' : '>');
}
}
-
- printf("\n");
-
+ if (show_mem || show_details)
+ printf("\n");
return 0;
}
@@ -2505,6 +2479,7 @@ static int unix_show(struct filter *f)
if (!(u = malloc(sizeof(*u))))
break;
u->name = NULL;
+ u->peer_name = NULL;
if (sscanf(buf, "%x: %x %x %x %x %x %d %s",
&u->peer, &u->rq, &u->wq, &flags, &u->type,
@@ -2544,7 +2519,7 @@ static int unix_show(struct filter *f)
strcpy(u->name, name);
}
if (++cnt > MAX_UNIX_REMEMBER) {
- unix_list_print(list, f);
+ unix_stats_print(list, f);
unix_list_free(list);
list = NULL;
cnt = 0;
@@ -2552,7 +2527,7 @@ static int unix_show(struct filter *f)
}
fclose(fp);
if (list) {
- unix_list_print(list, f);
+ unix_stats_print(list, f);
unix_list_free(list);
list = NULL;
cnt = 0;
--
2.1.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox