* Re: [PATCH net] netlink: Don't reorder loads/stores before marking mmap netlink frame as available
From: Linus Torvalds @ 2014-12-18 19:13 UTC (permalink / raw)
To: Thomas Graf
Cc: Eric Dumazet, David Miller, Daniel Borkmann, Andy Lutomirski,
Patrick McHardy, Network Development
In-Reply-To: <20141218103026.GA16239@casper.infradead.org>
On Thu, Dec 18, 2014 at 2:30 AM, Thomas Graf <tgraf@suug.ch> wrote:
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index ef5f77b..2662821 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -550,9 +550,9 @@ static enum nl_mmap_status netlink_get_status(const struct nl_mmap_hdr *hdr)
> static void netlink_set_status(struct nl_mmap_hdr *hdr,
> enum nl_mmap_status status)
> {
> + smp_mb();
> hdr->nm_status = status;
> flush_dcache_page(pgvec_to_page(hdr));
> - smp_wmb();
> }
If this is performance-critical code (I have no idea), then a better
idea might be to use
smp_store_release(&hdr->nm_status, status);
instead of using explicit memory barriers. That's going to generally
be much cheaper than a memory barrier.
Linus
^ permalink raw reply
* Re: [PATCH net 2/2] geneve: Fix races between socket add and release.
From: Jesse Gross @ 2014-12-18 18:34 UTC (permalink / raw)
To: Thomas Graf; +Cc: David Miller, netdev, Andy Zhou, Stephen Hemminger
In-Reply-To: <20141217211513.GF28766@casper.infradead.org>
On Wed, Dec 17, 2014 at 1:15 PM, Thomas Graf <tgraf@suug.ch> wrote:
> On 12/17/14 at 10:48am, Jesse Gross wrote:
>> I generally agree (with the exception of kfree_rcu() - I believe that
>> is still needed since incoming packets reference it using RCU).
>
> I didn't inspect this in full detail but seems like the data path
> should only care about gs->sock which is properly refcnt'ed.
The data path (if you include the consumer in OVS), goes from sk to gs
to gs->rcv_data so it does care about the stuff that is being freed
here.
>> for destroying the socket. This was added by Stephen in "vxlan: listen
>> on multiple ports" but it's not obvious to me what problem it is
>> trying to avoid and I don't see a comment. If possible, it would be
>> nice to simplify this as well if the issue doesn't apply to Geneve.
>
> I don't have an explanation for that either. Each entry on the
> vni_list[] takes a vs->refcnt.
I don't think the workqueue is about refcounting - it looks to me like
the issue that is being avoided is either that RTNL is held and a
lower layer tries to take it again or another lock that nests in the
opposite way with RTNL. However, looking through the code, the problem
wasn't obvious to me.
^ permalink raw reply
* Re: [RFC PATCH net-next v2 1/1] net: Support for switch port configuration
From: Roopa Prabhu @ 2014-12-18 18:14 UTC (permalink / raw)
To: Varlese, Marco
Cc: netdev@vger.kernel.org, Fastabend, John R, Thomas Graf,
Jiri Pirko, sfeldma@gmail.com, linux-kernel@vger.kernel.org
In-Reply-To: <C4896FB061E7DE4AAC93031BDCA044B104ADA310@IRSMSX108.ger.corp.intel.com>
On 12/18/14, 10:02 AM, Varlese, Marco wrote:
> Removed unnecessary content for ease of reading...
>
>>>>>>> +/* Switch Port Attributes section */
>>>>>>> +
>>>>>>> +enum {
>>>>>>> + IFLA_ATTR_UNSPEC,
>>>>>>> + IFLA_ATTR_LEARNING,
>>>>>> Any reason you want learning here ?. This is covered as part of
>>>>>> the bridge setlink attributes.
>>>>>>
>>>>> Yes, because the user may _not_ want to go through a bridge
>>>>> interface
>>>> necessarily.
>>>> But, the bridge setlink/getlink interface was changed to accommodate
>> 'self'
>>>> for exactly such cases.
>>>> I kind of understand your case for the other attributes (these are
>>>> per port settings that switch asics provide).
>>>>
>>>> However, i don't understand the reason to pull in bridge attributes here.
>>>>
>>> Maybe, I am missing something so you might help. The learning attribute -
>> in my case - it is like all other attributes: a port attribute (as you said, port
>> settings that the switch provides per port).
>>> So, what I was saying is "why the user shall go through a bridge to configure
>> the learning attribute"? From my perspective, it is as any other attribute and
>> as such configurable on the port.
>>
>> Thinking about this some more, i don't see why any of these attributes
>> (except loopback. I dont understand the loopback attribute) cant be part of
>> the birdge port attributes.
>>
>> With this we will end up adding l2 attributes in two places: the general link
>> attributes and bridge attributes.
>>
>> And since we have gone down the path of using ndo_bridge_setlink/getlink
>> with 'self'....we should stick to that for all l2 attributes.
>>
>> The idea of overloading ndo_bridge_set/getlink, was to have the same set of
>> attributes but support both cases where the user wants to go through the
>> bridge driver or directly to the switch port driver. So, you are not really going
>> through the bridge driver if you use 'self' and ndo_bridge_setlink/getlink.
>>
> Roopa, one of the comments I got from Thomas Graf on my v1 patch was that your patch and mine were supplementary ("I think Roopa's patches are supplementary. Not all switchdev users will be backed with a Linux Bridge. I therefore welcome your patches very much")... I also understood by others that the patch made sense for the same reason. I simply do not understand why these attributes (and maybe others in the future) could not be configured directly on a standard port but have to go through a bridge.
>
ok, i am very confused in that case. The whole moving of bridge
attributes from the bridge driver to rtnetlink.c was to make the bridge
attributes accessible to any driver who wants to set l2/bridge
attributes on their switch ports. So, its unclear to me why we are doing
this parallel thing again.
This move to rtnetlink.c was done during the recent rocker support. so,
maybe scott/jiri can elaborate more.
^ permalink raw reply
* RE: [RFC PATCH net-next v2 1/1] net: Support for switch port configuration
From: Varlese, Marco @ 2014-12-18 18:02 UTC (permalink / raw)
To: Roopa Prabhu
Cc: netdev@vger.kernel.org, Fastabend, John R, Thomas Graf,
Jiri Pirko, sfeldma@gmail.com, linux-kernel@vger.kernel.org
In-Reply-To: <549313B8.6050102@cumulusnetworks.com>
Removed unnecessary content for ease of reading...
> >>>>> +/* Switch Port Attributes section */
> >>>>> +
> >>>>> +enum {
> >>>>> + IFLA_ATTR_UNSPEC,
> >>>>> + IFLA_ATTR_LEARNING,
> >>>> Any reason you want learning here ?. This is covered as part of
> >>>> the bridge setlink attributes.
> >>>>
> >>> Yes, because the user may _not_ want to go through a bridge
> >>> interface
> >> necessarily.
> >> But, the bridge setlink/getlink interface was changed to accommodate
> 'self'
> >> for exactly such cases.
> >> I kind of understand your case for the other attributes (these are
> >> per port settings that switch asics provide).
> >>
> >> However, i don't understand the reason to pull in bridge attributes here.
> >>
> > Maybe, I am missing something so you might help. The learning attribute -
> in my case - it is like all other attributes: a port attribute (as you said, port
> settings that the switch provides per port).
> > So, what I was saying is "why the user shall go through a bridge to configure
> the learning attribute"? From my perspective, it is as any other attribute and
> as such configurable on the port.
>
> Thinking about this some more, i don't see why any of these attributes
> (except loopback. I dont understand the loopback attribute) cant be part of
> the birdge port attributes.
>
> With this we will end up adding l2 attributes in two places: the general link
> attributes and bridge attributes.
>
> And since we have gone down the path of using ndo_bridge_setlink/getlink
> with 'self'....we should stick to that for all l2 attributes.
>
> The idea of overloading ndo_bridge_set/getlink, was to have the same set of
> attributes but support both cases where the user wants to go through the
> bridge driver or directly to the switch port driver. So, you are not really going
> through the bridge driver if you use 'self' and ndo_bridge_setlink/getlink.
>
Roopa, one of the comments I got from Thomas Graf on my v1 patch was that your patch and mine were supplementary ("I think Roopa's patches are supplementary. Not all switchdev users will be backed with a Linux Bridge. I therefore welcome your patches very much")... I also understood by others that the patch made sense for the same reason. I simply do not understand why these attributes (and maybe others in the future) could not be configured directly on a standard port but have to go through a bridge.
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: changes for wireless
From: John W. Linville @ 2014-12-18 17:58 UTC (permalink / raw)
To: David Miller
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20141218.124231.114306102839964827.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Thu, Dec 18, 2014 at 12:42:31PM -0500, David Miller wrote:
> From: "John W. Linville" <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Date: Wed, 17 Dec 2014 12:07:05 -0500
>
> > http://marc.info/?l=linux-wireless&m=141883202530292&w=2
> >
> > This makes it official... :-)
> >
> > Signed-off-by: John W. Linville <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
>
> Applied, and thank you for all of your tireless work over the many
> years that you have maintained the wireless code.
>
> You have undoubtedly made my life easier and made things run much more
> smoothly than they otherwise would have, and for that I am eternally
> grateful.
>
> Thank you!
Thanks, Dave and everyone else, for the kind words of appreciation.
They are warmly received!
John
--
John W. Linville Someday the world will need a hero, and you
linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org might be all we have. Be ready.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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: [PATCHv1 net] xen-netback: support frontends without feature-rx-notify again
From: David Vrabel @ 2014-12-18 17:55 UTC (permalink / raw)
To: David Miller; +Cc: netdev, xen-devel, ian.campbell, wei.liu2
In-Reply-To: <20141218.125009.1837191564956948256.davem@davemloft.net>
On 18/12/14 17:50, David Miller wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> Date: Thu, 18 Dec 2014 11:13:06 +0000
>
>> Commit bc96f648df1bbc2729abbb84513cf4f64273a1f1 (xen-netback: make
>> feature-rx-notify mandatory) incorrectly assumed that there were no
>> frontends in use that did not support this feature. But the frontend
>> driver in MiniOS does not and since this is used by (qemu) stubdoms,
>> these stopped working.
>>
>> Netback sort of works as-is in this mode except:
>>
>> - If there are no Rx requests and the internal Rx queue fills, only
>> the drain timeout will wake the thread. The default drain timeout
>> of 10 s would give unacceptable pauses.
>>
>> - If an Rx stall was detected and the internal Rx queue is drained,
>> then the Rx thread would never wake.
>>
>> Handle these two cases (when feature-rx-notify is disabled) by:
>>
>> - Reducing the drain timeout to 30 ms.
>>
>> - Disabling Rx stall detection.
>>
>> Reported-by: John <jw@nuclearfallout.net>
>> Tested-by: John <jw@nuclearfallout.net>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>
> Applied, and I assume that 3.18 -stable needs this as well?
Yes, please. Without it, netback just doesn't work with an important
use case.
Thanks.
David
^ permalink raw reply
* Re: [PATCH net] be2net: Fix incorrect setting of tunnel offload flag in netdev features
From: David Miller @ 2014-12-18 17:51 UTC (permalink / raw)
To: sriharsha.basavapatna; +Cc: netdev
In-Reply-To: <1418963418-18792-1-git-send-email-sriharsha.basavapatna@emulex.com>
From: Sriharsha Basavapatna <sriharsha.basavapatna@emulex.com>
Date: Fri, 19 Dec 2014 10:00:18 +0530
> An earlier commit to resolve an issue with encapsulation offloads missed
> setting a bit in the outer netdev features flag. This results in loss of TSO
> feature on a VxLAN interface.
>
> Fixes: 630f4b70 ("Export tunnel offloads only when a VxLAN tunnel is created")
>
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@emulex.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net] bnx2x: fix typos in "configure"
From: David Miller @ 2014-12-18 17:50 UTC (permalink / raw)
To: jbenc; +Cc: netdev, ariel.elior
In-Reply-To: <fcb186d22a0f6a2e3b07a6a88baebbada6742003.1418889837.git.jbenc@redhat.com>
From: Jiri Benc <jbenc@redhat.com>
Date: Thu, 18 Dec 2014 09:04:35 +0100
> Noticed when debugging ptp.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
Applied, thanks Jiri.
^ permalink raw reply
* Re: [PATCHv1 net] xen-netback: support frontends without feature-rx-notify again
From: David Miller @ 2014-12-18 17:50 UTC (permalink / raw)
To: david.vrabel; +Cc: netdev, xen-devel, ian.campbell, wei.liu2
In-Reply-To: <1418901186-14478-1-git-send-email-david.vrabel@citrix.com>
From: David Vrabel <david.vrabel@citrix.com>
Date: Thu, 18 Dec 2014 11:13:06 +0000
> Commit bc96f648df1bbc2729abbb84513cf4f64273a1f1 (xen-netback: make
> feature-rx-notify mandatory) incorrectly assumed that there were no
> frontends in use that did not support this feature. But the frontend
> driver in MiniOS does not and since this is used by (qemu) stubdoms,
> these stopped working.
>
> Netback sort of works as-is in this mode except:
>
> - If there are no Rx requests and the internal Rx queue fills, only
> the drain timeout will wake the thread. The default drain timeout
> of 10 s would give unacceptable pauses.
>
> - If an Rx stall was detected and the internal Rx queue is drained,
> then the Rx thread would never wake.
>
> Handle these two cases (when feature-rx-notify is disabled) by:
>
> - Reducing the drain timeout to 30 ms.
>
> - Disabling Rx stall detection.
>
> Reported-by: John <jw@nuclearfallout.net>
> Tested-by: John <jw@nuclearfallout.net>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Applied, and I assume that 3.18 -stable needs this as well?
Thanks.
^ permalink raw reply
* Re: [PATCH 01/10] core: Split out UFO6 support
From: Michael S. Tsirkin @ 2014-12-18 17:50 UTC (permalink / raw)
To: Vlad Yasevich
Cc: netdev, Vladislav Yasevich, virtualization, stefanha, ben,
David Miller
In-Reply-To: <20141218173546.GB5425@redhat.com>
On Thu, Dec 18, 2014 at 07:35:46PM +0200, Michael S. Tsirkin wrote:
> > > We should either generate our own ID,
> > > like we always did, or make sure we don't accept
> > > these packets.
> > > Second option is almost sure to break userspace,
> > > so it seems we must do the first one.
> > >
> >
> > Right. This was missing from packet sockets. I can fix it.
> >
> > -vlad
>
> Also, this can't be a patch on top, since we don't
> want bisect to give us configurations which
> can BUG().
So how doing this in stages:
1. add helper that checks skb GSO type.
If it is SKB_GSO_UDP, check for IPv6, and
generate the fragment ID.
Call this helper in
- virtio net rx path
- tun rx path (get user)
- macvtap tx patch (get user)
- packet socket tx patch (get user)
2. Put back UFO flag everywhere: virtio,tun,macvtap,packet,
since we can handle IPv6 now even if it's suboptimal.
Above two seem like smalling patches, appropriate for stable.
Next, work on a new feature to pass
fragment ID in virtio header:
3. split UFO/UFO6 as you did here, but add code
in above 4 places to convert UDP to UDP6,
additionally, add code in
- virtio net tx path
- tun tx path (get user)
- macvtap rx patch (put user)
- packet socket rx patch (put user)
to convert UDP6 to UDP.
step 3 needs to be bisect-clean, somehow.
4. add new field in header, new feature bit for virtio net to gate it,
new ioctls to tun,macvtap,packet socket.
These two are more like optimizations, so not stable material.
--
MST
^ permalink raw reply
* Re: [RFC PATCH net-next v2 1/1] net: Support for switch port configuration
From: Roopa Prabhu @ 2014-12-18 17:49 UTC (permalink / raw)
To: Varlese, Marco
Cc: netdev@vger.kernel.org, Fastabend, John R, Thomas Graf,
Jiri Pirko, sfeldma@gmail.com, linux-kernel@vger.kernel.org
In-Reply-To: <C4896FB061E7DE4AAC93031BDCA044B104ADA16F@IRSMSX108.ger.corp.intel.com>
On 12/18/14, 9:25 AM, Varlese, Marco wrote:
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-
>> owner@vger.kernel.org] On Behalf Of Roopa Prabhu
>> Sent: Thursday, December 18, 2014 3:16 PM
>> To: Varlese, Marco
>> Cc: netdev@vger.kernel.org; Fastabend, John R; Thomas Graf; Jiri Pirko;
>> sfeldma@gmail.com; linux-kernel@vger.kernel.org
>> Subject: Re: [RFC PATCH net-next v2 1/1] net: Support for switch port
>> configuration
>>
>> On 12/18/14, 6:55 AM, Varlese, Marco wrote:
>>>> -----Original Message-----
>>>> From: Roopa Prabhu [mailto:roopa@cumulusnetworks.com]
>>>> Sent: Thursday, December 18, 2014 2:45 PM
>>>> To: Varlese, Marco
>>>> Cc: netdev@vger.kernel.org; Fastabend, John R; Thomas Graf; Jiri
>>>> Pirko; sfeldma@gmail.com; linux-kernel@vger.kernel.org
>>>> Subject: Re: [RFC PATCH net-next v2 1/1] net: Support for switch port
>>>> configuration
>>>>
>>>> On 12/18/14, 3:29 AM, Varlese, Marco wrote:
>>>>> From: Marco Varlese <marco.varlese@intel.com>
>>>>>
>>>>> Switch hardware offers a list of attributes that are configurable on
>>>>> a per port basis.
>>>>> This patch provides a mechanism to configure switch ports by adding
>>>>> an NDO for setting specific values to specific attributes.
>>>>> There will be a separate patch that adds the "get" functionality via
>>>>> another NDO and another patch that extends iproute2 to call the two
>>>>> new NDOs.
>>>>>
>>>>> Signed-off-by: Marco Varlese <marco.varlese@intel.com>
>>>>> ---
>>>>> include/linux/netdevice.h | 5 ++++
>>>>> include/uapi/linux/if_link.h | 15 ++++++++++++
>>>>> net/core/rtnetlink.c | 54
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 74 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>> index c31f74d..4881c7b 100644
>>>>> --- a/include/linux/netdevice.h
>>>>> +++ b/include/linux/netdevice.h
>>>>> @@ -1027,6 +1027,9 @@ typedef u16 (*select_queue_fallback_t)(struct
>>>> net_device *dev,
>>>>> * int (*ndo_switch_port_stp_update)(struct net_device *dev, u8
>> state);
>>>>> * Called to notify switch device port of bridge port STP
>>>>> * state change.
>>>>> + * int (*ndo_switch_port_set_cfg)(struct net_device *dev,
>>>>> + * u32 attr, u64 value);
>>>>> + * Called to set specific switch ports attributes.
>>>>> */
>>>>> struct net_device_ops {
>>>>> int (*ndo_init)(struct net_device *dev);
>>>>> @@ -1185,6 +1188,8 @@ struct net_device_ops {
>>>>> struct
>>>> netdev_phys_item_id *psid);
>>>>> int (*ndo_switch_port_stp_update)(struct
>>>> net_device *dev,
>>>>> u8 state);
>>>>> + int (*ndo_switch_port_set_cfg)(struct
>>>> net_device *dev,
>>>>> + u32 attr, u64 value);
>>>>> #endif
>>>>> };
>>>>>
>>>>> diff --git a/include/uapi/linux/if_link.h
>>>>> b/include/uapi/linux/if_link.h index f7d0d2d..19cb51a 100644
>>>>> --- a/include/uapi/linux/if_link.h
>>>>> +++ b/include/uapi/linux/if_link.h
>>>>> @@ -146,6 +146,7 @@ enum {
>>>>> IFLA_PHYS_PORT_ID,
>>>>> IFLA_CARRIER_CHANGES,
>>>>> IFLA_PHYS_SWITCH_ID,
>>>>> + IFLA_SWITCH_PORT_CFG,
>>>>> __IFLA_MAX
>>>>> };
>>>>>
>>>>> @@ -603,4 +604,18 @@ enum {
>>>>>
>>>>> #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
>>>>>
>>>>> +/* Switch Port Attributes section */
>>>>> +
>>>>> +enum {
>>>>> + IFLA_ATTR_UNSPEC,
>>>>> + IFLA_ATTR_LEARNING,
>>>> Any reason you want learning here ?. This is covered as part of the
>>>> bridge setlink attributes.
>>>>
>>> Yes, because the user may _not_ want to go through a bridge interface
>> necessarily.
>> But, the bridge setlink/getlink interface was changed to accommodate 'self'
>> for exactly such cases.
>> I kind of understand your case for the other attributes (these are per port
>> settings that switch asics provide).
>>
>> However, i don't understand the reason to pull in bridge attributes here.
>>
> Maybe, I am missing something so you might help. The learning attribute - in my case - it is like all other attributes: a port attribute (as you said, port settings that the switch provides per port).
> So, what I was saying is "why the user shall go through a bridge to configure the learning attribute"? From my perspective, it is as any other attribute and as such configurable on the port.
Thinking about this some more, i don't see why any of these attributes
(except loopback. I dont understand the loopback attribute)
cant be part of the birdge port attributes.
With this we will end up adding l2 attributes in two places: the general
link attributes and bridge attributes.
And since we have gone down the path of using ndo_bridge_setlink/getlink
with 'self'....we should stick to that for all l2 attributes.
The idea of overloading ndo_bridge_set/getlink, was to have the same set
of attributes but support both cases where the user wants to go through
the bridge driver or directly to the switch port driver. So, you are not
really going through the bridge driver if you use 'self' and
ndo_bridge_setlink/getlink.
>>>>> + IFLA_ATTR_LOOPBACK,
>>>>> + IFLA_ATTR_BCAST_FLOODING,
>>>>> + IFLA_ATTR_UCAST_FLOODING,
>>>>> + IFLA_ATTR_MCAST_FLOODING,
>>>>> + __IFLA_ATTR_MAX
>>>>> +};
>>>>> +
>>>>> +#define IFLA_ATTR_MAX (__IFLA_ATTR_MAX - 1)
>>>>> +
>>>>> #endif /* _UAPI_LINUX_IF_LINK_H */ diff --git
>>>>> a/net/core/rtnetlink.c b/net/core/rtnetlink.c index
>>>>> eaa057f..c0d1c45 100644
>>>>> --- a/net/core/rtnetlink.c
>>>>> +++ b/net/core/rtnetlink.c
>>>>> @@ -1389,6 +1389,46 @@ static int validate_linkmsg(struct net_device
>>>> *dev, struct nlattr *tb[])
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +#ifdef CONFIG_NET_SWITCHDEV
>>>>> +static int do_setswcfg(struct net_device *dev, struct nlattr *attr) {
>>>>> + int rem, err = -EINVAL;
>>>>> + struct nlattr *v;
>>>>> + const struct net_device_ops *ops = dev->netdev_ops;
>>>>> +
>>>>> + nla_for_each_nested(v, attr, rem) {
>>>>> + u32 op = nla_type(v);
>>>>> + u64 value = 0;
>>>>> +
>>>>> + switch (op) {
>>>>> + case IFLA_ATTR_LEARNING:
>>>>> + case IFLA_ATTR_LOOPBACK:
>>>>> + case IFLA_ATTR_BCAST_FLOODING:
>>>>> + case IFLA_ATTR_UCAST_FLOODING:
>>>>> + case IFLA_ATTR_MCAST_FLOODING: {
>>>>> + if (nla_len(v) < sizeof(value)) {
>>>>> + err = -EINVAL;
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + value = nla_get_u64(v);
>>>>> + err = ops->ndo_switch_port_set_cfg(dev,
>>>>> + op,
>>>>> + value);
>>>>> + break;
>>>>> + }
>>>>> + default:
>>>>> + err = -EINVAL;
>>>>> + break;
>>>>> + }
>>>>> + if (err)
>>>>> + break;
>>>>> + }
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +#endif
>>>>> +
>>>>> static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
>>>>> {
>>>>> int rem, err = -EINVAL;
>>>>> @@ -1740,6 +1780,20 @@ static int do_setlink(const struct sk_buff *skb,
>>>>> status |= DO_SETLINK_NOTIFY;
>>>>> }
>>>>> }
>>>>> +#ifdef CONFIG_NET_SWITCHDEV
>>>>> + if (tb[IFLA_SWITCH_PORT_CFG]) {
>>>>> + err = -EOPNOTSUPP;
>>>>> + if (!ops->ndo_switch_port_set_cfg)
>>>>> + goto errout;
>>>>> + if (!ops->ndo_switch_parent_id_get)
>>>>> + goto errout;
>>>>> + err = do_setswcfg(dev, tb[IFLA_SWITCH_PORT_CFG]);
>>>>> + if (err < 0)
>>>>> + goto errout;
>>>>> +
>>>>> + status |= DO_SETLINK_NOTIFY;
>>>>> + }
>>>>> +#endif
>>>>> err = 0;
>>>>>
>>>>> errout:
>>> --
>>> 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
>> --
>> 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 net] enic: fix rx skb checksum
From: David Miller @ 2014-12-18 17:49 UTC (permalink / raw)
To: jay.vosburgh
Cc: eric.dumazet, _govind, netdev, ssujith, benve, jbenc, sassmann
In-Reply-To: <17951.1418924367@famine>
From: Jay Vosburgh <jay.vosburgh@canonical.com>
Date: Thu, 18 Dec 2014 09:39:27 -0800
> @@ -1694,6 +1694,7 @@ int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
>
> skb_scrub_packet(skb, true);
> skb->protocol = eth_type_trans(skb, dev);
> + skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
>
> return 0;
> }
>
> I'd very much appreciate comments from someone who knows the
> checksum logic better than I do as to whether this is a reasonable thing
> to do.
This looks quite reasonable to me.
^ permalink raw reply
* Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks
From: SF Markus Elfring @ 2014-12-18 17:44 UTC (permalink / raw)
To: David Miller
Cc: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet,
linux-kernel, kernel-janitors, Julia Lawall
In-Reply-To: <20141218.122556.2148647081075440879.davem@davemloft.net>
>> Now I find still that your data reorgansisation wish can not be resolved
>> in a simple way.
>
> I'm saying to leave the code alone.
It seems that there might be a misunderstanding between us.
> If it goes:
>
> var = foo_that_returns_ptr_err()
> if (IS_ERR(var))
> return PTR_ERR(var);
>
> p->bar = var;
>
> or whatever, simply keep it that way!
A simple return was not used by the mppe_alloc() function so far because
a bit of memory clean-up will also be useful after error detection,
won't it?
> I'm not engaging in this conversation any further, you have already
> consumed way too much of my limited time on this incredibly trivial matter.
It can occasionally happen that a safe clarification of specific implementation
details will need more efforts than you would like to invest at the moment.
Regards,
Markus
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: changes for wireless
From: David Miller @ 2014-12-18 17:42 UTC (permalink / raw)
To: linville; +Cc: netdev, linux-wireless
In-Reply-To: <1418836025-9035-1-git-send-email-linville@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Wed, 17 Dec 2014 12:07:05 -0500
> http://marc.info/?l=linux-wireless&m=141883202530292&w=2
>
> This makes it official... :-)
>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
Applied, and thank you for all of your tireless work over the many
years that you have maintained the wireless code.
You have undoubtedly made my life easier and made things run much more
smoothly than they otherwise would have, and for that I am eternally
grateful.
Thank you!
^ permalink raw reply
* Re: [PATCH net] cxgb4: Fix decoding QSA module for ethtool get settings
From: David Miller @ 2014-12-18 17:39 UTC (permalink / raw)
To: hariprasad; +Cc: netdev, leedom, nirranjan
In-Reply-To: <1418817960-17277-1-git-send-email-hariprasad@chelsio.com>
From: Hariprasad Shenai <hariprasad@chelsio.com>
Date: Wed, 17 Dec 2014 17:36:00 +0530
> QSA module was getting decoded as QSFP module in ethtool get settings, this
> patch fixes it.
>
> Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net] enic: fix rx skb checksum
From: Jay Vosburgh @ 2014-12-18 17:39 UTC (permalink / raw)
To: Eric Dumazet
Cc: Govindarajulu Varadarajan, davem, netdev, ssujith, benve,
Jiri Benc, Stefan Assmann
In-Reply-To: <1418920017.9773.80.camel@edumazet-glaptop2.roam.corp.google.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
>On Thu, 2014-12-18 at 15:58 +0530, Govindarajulu Varadarajan wrote:
>> Hardware always provides compliment of IP pseudo checksum. Stack expects
>> whole packet checksum without pseudo checksum if CHECKSUM_COMPLETE is set.
>>
>> This causes checksum error in nf & ovs.
>>
>> kernel: qg-19546f09-f2: hw csum failure
>> kernel: CPU: 9 PID: 0 Comm: swapper/9 Tainted: GF O-------------- 3.10.0-123.8.1.el7.x86_64 #1
>> kernel: Hardware name: Cisco Systems Inc UCSB-B200-M3/UCSB-B200-M3, BIOS B200M3.2.2.3.0.080820141339 08/08/2014
>> kernel: ffff881218f40000 df68243feb35e3a8 ffff881237a43ab8 ffffffff815e237b
>> kernel: ffff881237a43ad0 ffffffff814cd4ca ffff8829ec71eb00 ffff881237a43af0
>> kernel: ffffffff814c6232 0000000000000286 ffff8829ec71eb00 ffff881237a43b00
>> kernel: Call Trace:
>> kernel: <IRQ> [<ffffffff815e237b>] dump_stack+0x19/0x1b
>> kernel: [<ffffffff814cd4ca>] netdev_rx_csum_fault+0x3a/0x40
>> kernel: [<ffffffff814c6232>] __skb_checksum_complete_head+0x62/0x70
>> kernel: [<ffffffff814c6251>] __skb_checksum_complete+0x11/0x20
>> kernel: [<ffffffff8155a20c>] nf_ip_checksum+0xcc/0x100
>> kernel: [<ffffffffa049edc7>] icmp_error+0x1f7/0x35c [nf_conntrack_ipv4]
>> kernel: [<ffffffff814cf419>] ? netif_rx+0xb9/0x1d0
>> kernel: [<ffffffffa040eb7b>] ? internal_dev_recv+0xdb/0x130 [openvswitch]
>> kernel: [<ffffffffa04c8330>] nf_conntrack_in+0xf0/0xa80 [nf_conntrack]
>> kernel: [<ffffffff81509380>] ? inet_del_offload+0x40/0x40
>> kernel: [<ffffffffa049e302>] ipv4_conntrack_in+0x22/0x30 [nf_conntrack_ipv4]
>> kernel: [<ffffffff815005ca>] nf_iterate+0xaa/0xc0
>> kernel: [<ffffffff81509380>] ? inet_del_offload+0x40/0x40
>> kernel: [<ffffffff81500664>] nf_hook_slow+0x84/0x140
>> kernel: [<ffffffff81509380>] ? inet_del_offload+0x40/0x40
>> kernel: [<ffffffff81509dd4>] ip_rcv+0x344/0x380
>>
>> Hardware verifies IP & tcp/udp header checksum but does not provide payload
>> checksum, use CHECKSUM_UNNECESSARY. Set it only if its valid IP tcp/udp packet.
>>
>> Cc: Jiri Benc <jbenc@redhat.com>
>> Cc: Stefan Assmann <sassmann@redhat.com>
>> Reported-by: Sunil Choudhary <schoudha@redhat.com>
>> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
>> ---
>> drivers/net/ethernet/cisco/enic/enic_main.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
>> index 868d0f6..705f334 100644
>> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
>> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
>> @@ -1060,10 +1060,14 @@ static void enic_rq_indicate_buf(struct vnic_rq *rq,
>> PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
>> }
>>
>> - if ((netdev->features & NETIF_F_RXCSUM) && !csum_not_calc) {
>> - skb->csum = htons(checksum);
>> - skb->ip_summed = CHECKSUM_COMPLETE;
>> - }
>> + /* Hardware does not provide whole packet checksum. It only
>> + * provides pseudo checksum. Since hw validates the packet
>> + * checksum but not provide us the checksum value. use
>> + * CHECSUM_UNNECESSARY.
>> + */
>> + if ((netdev->features & NETIF_F_RXCSUM) && tcp_udp_csum_ok &&
>> + ipv4_csum_ok)
>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>>
>> if (vlan_stripped)
>> __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tci);
>
>Hmm.. this looks like CHECKSUM_COMPLETE could be kept for tunneling
>cases.
>
>Check commit f8c6455bb04b944e ("net/mlx4_en: Extend checksum offloading
>by CHECKSUM COMPLETE") for a start.
I've actually been looking into this "hw csum failure" (as it
appears with OVS and VXLAN) the last couple of days, and, at least on
the sky2 hardware I have, the problem doesn't appear to be the
hardware's CHECKSUM_COMPLETE checksumming. Instead, it appears that
when the encapsulated packet is decapsulated and forwarded to the guest
or container, the checksum isn't updated for the encapsulated ethernet
header. eth_type_trans does a pull for the ethernet header, but
skb->csum isn't updated.
I'm testing this patch, and it resolves the problem for me, but
I'm not yet entirely sure whether breaks something else:
diff --git a/net/core/dev.c b/net/core/dev.c
index f411c28..df755e5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1694,6 +1694,7 @@ int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
skb_scrub_packet(skb, true);
skb->protocol = eth_type_trans(skb, dev);
+ skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
return 0;
}
I'd very much appreciate comments from someone who knows the
checksum logic better than I do as to whether this is a reasonable thing
to do.
I've also not tested it on enic hardware. Govindarajulu, would
you be able to test this against the unmodified enic driver and see if
it resolves the problem for you?
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply related
* Re: [PATCH net 2/2] geneve: Fix races between socket add and release.
From: David Miller @ 2014-12-18 17:38 UTC (permalink / raw)
To: jesse; +Cc: netdev, azhou
In-Reply-To: <1418783132-99230-2-git-send-email-jesse@nicira.com>
From: Jesse Gross <jesse@nicira.com>
Date: Tue, 16 Dec 2014 18:25:32 -0800
> Currently, searching for a socket to add a reference to is not
> synchronized with deletion of sockets. This can result in use
> after free if there is another operation that is removing a
> socket at the same time. Solving this requires both holding the
> appropriate lock and checking the refcount to ensure that it
> has not already hit zero.
>
> Inspired by a related (but not exactly the same) issue in the
> VXLAN driver.
>
> Fixes: 0b5e8b8e ("net: Add Geneve tunneling protocol driver")
> CC: Andy Zhou <azhou@nicira.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>
Applied.
^ permalink raw reply
* Re: [PATCH net 1/2] geneve: Remove socket and offload handlers at destruction.
From: David Miller @ 2014-12-18 17:38 UTC (permalink / raw)
To: jesse; +Cc: netdev, azhou
In-Reply-To: <1418783132-99230-1-git-send-email-jesse@nicira.com>
From: Jesse Gross <jesse@nicira.com>
Date: Tue, 16 Dec 2014 18:25:31 -0800
> Sockets aren't currently removed from the the global list when
> they are destroyed. In addition, offload handlers need to be cleaned
> up as well.
>
> Fixes: 0b5e8b8e ("net: Add Geneve tunneling protocol driver")
> CC: Andy Zhou <azhou@nicira.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] netlink: Don't reorder loads/stores before marking mmap netlink frame as available
From: David Miller @ 2014-12-18 17:36 UTC (permalink / raw)
To: tgraf; +Cc: eric.dumazet, dborkman, luto, torvalds, kaber, netdev
In-Reply-To: <20141218103026.GA16239@casper.infradead.org>
From: Thomas Graf <tgraf@suug.ch>
Date: Thu, 18 Dec 2014 10:30:26 +0000
> Each mmap Netlink frame contains a status field which indicates
> whether the frame is unused, reserved, contains data or needs to
> be skipped. Both loads and stores may not be reordeded and must
> complete before the status field is changed and another CPU might
> pick up the frame for use. Use an smp_mb() to cover needs of both
> types of callers to netlink_set_status(), callers which have been
> reading data frame from the frame, and callers which have been
> filling or releasing and thus writing to the frame.
>
> - Example code path requiring a smp_rmb():
> memcpy(skb->data, (void *)hdr + NL_MMAP_HDRLEN, hdr->nm_len);
> netlink_set_status(hdr, NL_MMAP_STATUS_UNUSED);
>
> - Example code path requiring a smp_wmb():
> hdr->nm_uid = from_kuid(sk_user_ns(sk), NETLINK_CB(skb).creds.uid);
> hdr->nm_gid = from_kgid(sk_user_ns(sk), NETLINK_CB(skb).creds.gid);
> netlink_frame_flush_dcache(hdr);
> netlink_set_status(hdr, NL_MMAP_STATUS_VALID);
>
> Fixes: f9c228 ("netlink: implement memory mapped recvmsg()")
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
Also applied and queued up for -stable, thanks Thomas.
^ permalink raw reply
* Re: Netlink mmap tx security?
From: David Miller @ 2014-12-18 17:36 UTC (permalink / raw)
To: tgraf; +Cc: dborkman, luto, torvalds, kaber, netdev
In-Reply-To: <20141217162749.GC28766@casper.infradead.org>
From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 17 Dec 2014 16:27:49 +0000
> On 12/17/14 at 12:58am, Daniel Borkmann wrote:
>> Fixes: 5fd96123ee19 ("netlink: implement memory mapped sendmsg()")
>> Acked-by: Daniel Borkmann <dborkman@redhat.com>
>
> Nothing to add to Daniel's excellent feedback.
>
> Acked-by: Thomas Graf <tgraf@suug.ch>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH 01/10] core: Split out UFO6 support
From: Michael S. Tsirkin @ 2014-12-18 17:35 UTC (permalink / raw)
To: Vlad Yasevich
Cc: netdev, Vladislav Yasevich, virtualization, stefanha, ben,
David Miller
In-Reply-To: <5492EC4F.5080303@redhat.com>
On Thu, Dec 18, 2014 at 10:01:35AM -0500, Vlad Yasevich wrote:
> On 12/18/2014 02:54 AM, Michael S. Tsirkin wrote:
> > Cc Dave, pls remember to do this next time otherwise
> > your patches won't get merged :)
> >
> > On Wed, Dec 17, 2014 at 06:31:50PM -0500, Vlad Yasevich wrote:
> >> On 12/17/2014 05:45 PM, Michael S. Tsirkin wrote:
> >>> On Wed, Dec 17, 2014 at 01:20:46PM -0500, Vladislav Yasevich wrote:
> >>>> Split IPv6 support for UFO into its own feature similiar to TSO.
> >>>> This will later allow us to re-enable UFO support for virtio-net
> >>>> devices.
> >>>>
> >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> >>>> ---
> >>>> include/linux/netdev_features.h | 7 +++++--
> >>>> include/linux/netdevice.h | 1 +
> >>>> include/linux/skbuff.h | 1 +
> >>>> net/core/dev.c | 35 +++++++++++++++++++----------------
> >>>> net/core/ethtool.c | 2 +-
> >>>> 5 files changed, 27 insertions(+), 19 deletions(-)
> >>>>
> >>>> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> >>>> index dcfdecb..a078945 100644
> >>>> --- a/include/linux/netdev_features.h
> >>>> +++ b/include/linux/netdev_features.h
> >>>> @@ -48,8 +48,9 @@ enum {
> >>>> NETIF_F_GSO_UDP_TUNNEL_BIT, /* ... UDP TUNNEL with TSO */
> >>>> NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT,/* ... UDP TUNNEL with TSO & CSUM */
> >>>> NETIF_F_GSO_MPLS_BIT, /* ... MPLS segmentation */
> >>>> + NETIF_F_UFO6_BIT, /* ... UDPv6 fragmentation */
> >>>> /**/NETIF_F_GSO_LAST = /* last bit, see GSO_MASK */
> >>>> - NETIF_F_GSO_MPLS_BIT,
> >>>> + NETIF_F_UFO6_BIT,
> >>>>
> >>>> NETIF_F_FCOE_CRC_BIT, /* FCoE CRC32 */
> >>>> NETIF_F_SCTP_CSUM_BIT, /* SCTP checksum offload */
> >>>> @@ -109,6 +110,7 @@ enum {
> >>>> #define NETIF_F_TSO_ECN __NETIF_F(TSO_ECN)
> >>>> #define NETIF_F_TSO __NETIF_F(TSO)
> >>>> #define NETIF_F_UFO __NETIF_F(UFO)
> >>>> +#define NETIF_F_UFO6 __NETIF_F(UFO6)
> >>>> #define NETIF_F_VLAN_CHALLENGED __NETIF_F(VLAN_CHALLENGED)
> >>>> #define NETIF_F_RXFCS __NETIF_F(RXFCS)
> >>>> #define NETIF_F_RXALL __NETIF_F(RXALL)
> >>>> @@ -141,7 +143,7 @@ enum {
> >>>>
> >>>> /* List of features with software fallbacks. */
> >>>> #define NETIF_F_GSO_SOFTWARE (NETIF_F_TSO | NETIF_F_TSO_ECN | \
> >>>> - NETIF_F_TSO6 | NETIF_F_UFO)
> >>>> + NETIF_F_TSO6 | NETIF_F_UFO | NETIF_F_UFO6)
> >>>>
> >>>> #define NETIF_F_GEN_CSUM NETIF_F_HW_CSUM
> >>>> #define NETIF_F_V4_CSUM (NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
> >>>> @@ -149,6 +151,7 @@ enum {
> >>>> #define NETIF_F_ALL_CSUM (NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
> >>>>
> >>>> #define NETIF_F_ALL_TSO (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
> >>>> +#define NETIF_F_ALL_UFO (NETIF_F_UFO | NETIF_F_UFO6)
> >>>>
> >>>> #define NETIF_F_ALL_FCOE (NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
> >>>> NETIF_F_FSO)
> >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >>>> index 74fd5d3..86af10a 100644
> >>>> --- a/include/linux/netdevice.h
> >>>> +++ b/include/linux/netdevice.h
> >>>> @@ -3559,6 +3559,7 @@ static inline bool net_gso_ok(netdev_features_t features, int gso_type)
> >>>> /* check flags correspondence */
> >>>> BUILD_BUG_ON(SKB_GSO_TCPV4 != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
> >>>> BUILD_BUG_ON(SKB_GSO_UDP != (NETIF_F_UFO >> NETIF_F_GSO_SHIFT));
> >>>> + BUILD_BUG_ON(SKB_GSO_UDP6 != (NETIF_F_UFO6 >> NETIF_F_GSO_SHIFT));
> >>>> BUILD_BUG_ON(SKB_GSO_DODGY != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
> >>>> BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
> >>>> BUILD_BUG_ON(SKB_GSO_TCPV6 != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
> >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >>>> index 6c8b6f6..8538b67 100644
> >>>> --- a/include/linux/skbuff.h
> >>>> +++ b/include/linux/skbuff.h
> >>>> @@ -372,6 +372,7 @@ enum {
> >>>>
> >>>> SKB_GSO_MPLS = 1 << 12,
> >>>>
> >>>> + SKB_GSO_UDP6 = 1 << 13
> >>>> };
> >>>>
> >>>> #if BITS_PER_LONG > 32
> >>>
> >>> So this implies anything getting GSO packets e.g.
> >>> from userspace now needs to check IP version to
> >>> set GSO type correctly.
> >>>
> >>> I think you missed some places that do this, e.g. af_packet
> >>> sockets.
> >>>
> >>
> >> I looked at af_packet sockets and they set this only in the event
> >> vnet header has been used with a GSO type. In this case, the user
> >> already knows the the type.
> >
> > Imagine you are receiving a packet:
> >
> > if (vnet_hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> > switch (vnet_hdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> > case VIRTIO_NET_HDR_GSO_TCPV4:
> > gso_type = SKB_GSO_TCPV4;
> > break;
> > case VIRTIO_NET_HDR_GSO_TCPV6:
> > gso_type = SKB_GSO_TCPV6;
> > break;
> > case VIRTIO_NET_HDR_GSO_UDP:
> > gso_type = SKB_GSO_UDP;
> > break;
> > default:
> > goto out_unlock;
> > }
> >
> > if (vnet_hdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)
> > gso_type |= SKB_GSO_TCP_ECN;
> >
> > if (vnet_hdr.gso_size == 0)
> > goto out_unlock;
> >
> > }
> >
> > we used to report UFO6 as SKB_GSO_UDP, we probably
> > should keep doing this, with your patch we select the
> > goto out_unlock path.
> >
> >
>
> No. The vnet_hdr.gso_type is still going to be VIRTIO_NET_HDR_GSO_UDP
> since the UDP6 version isn't defined yet. So, it will be marked as
> GSO_UDP.
I pasted the wrong snippet.
I meant this:
/* This is a hint as to how much should be * linear. */
vnet_hdr.hdr_len = __cpu_to_virtio16(false, skb_headlen(skb));
vnet_hdr.gso_size = __cpu_to_virtio16(false, sinfo->gso_size);
if (sinfo->gso_type & SKB_GSO_TCPV4)
vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (sinfo->gso_type & SKB_GSO_TCPV6)
vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
else if (sinfo->gso_type & SKB_GSO_UDP)
vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
else if (sinfo->gso_type & SKB_GSO_FCOE)
goto out_free;
else
BUG();
so if we get SKB_GSO_UDP we'll get BUG().
> This code most likely needs the same workaround as exists in tap and macvtap,
> i.e select the proxy fragment id and decide what to do with gso_type.
And fixup type to GSO_UDP6 while we are at it.
> >
> >> It is true that with this series af_packets now can't do IPv6 UFO
> >> since there is no VIRTIO_NET_HDR_GSO_UDPV6 yet.
> >
> > What do you mean by "do".
>
> What I mean is that AF_PACKET sockets currently do not do IPv6 UFO
> correctly, even after Ben's fixes to tap/macvtap. There is no
> proxy fragment id selection in af_packet case and we have the
> same problem Ben was trying address for tap/macvtap.
> > Are we talking about sending or receiving packets?
>
> I am talking about sending, see above.
>
> > You seem to conflate the two.
> >
> > We always discarded ID on RX.
> >
> > For tun, this is xmit, so just by saying "this device can
> > not do UFO" you will never get short packets.
> >
>
> You must mean long packets.
Yes.
> This is actually an issue I've been thinking
> about. With with your suggestion of switching the GSO type for legacy
> applications we end up with fragments for IPv6 traffic. As a result,
> legacy VMs will see a regression for large IPv6 datagrams.
I'm not sure what's meant by my suggestion here :)
It seems clear that legacy applications don't want to get IPv6
fragment IDs in virtio header. Either we pass them plain ethernet
packets or assume they are ok with discarding the IDs even
if we set GSO_UDP.
> >
> >>
> >> I suppose we could do something similar there as we do in tun code/macvtap code.
> >> If that's the case, it currently broken as well.
> >>
> >> -vlad
> >
> >
> > Broken is a big word.
> >
> > Let's stop conflating two directions.
>
> I am not and was talking only about af_packet as that is what you asked about.
> There is no tun/macvtap in play here. They are handled separately in their
> respective drivers.
>
> >
> > Here's the way I look at it:
> >
> > 1. Userspace doesn't have a way to get fragment IDs
> > from tun/macvtap/packet sockets.
> > Presumably, not all users need these IDs.
> > E.g. old guests clearly don't.
> >
> > We should either give them packets stripping the ID,
> > like we always did, or make sure they never get these packets.
> > Second option seems achievable for tun if we
> > never tell linux it can do UFO, but I don't see
> > how to do it for macvtap and packet socket.
> >
>
> macvtap is slightly problematic, but doable with some tricks.
> packet socket is an interesting problem. The only way
> an af_packet socket can receive an skb marked SKB_GSO_UDPV6
> is if someone else on the host sent it (like another guest).
Or if a NIC set it.
> Since there is are no feature or vnet header length negotiations,
> it is impossible to tell if an application using this af_packet
> socket is capable of processing VIRTIO_NET_HDR_GSO_UDPV6
> type (yet to be defined).
>
> So, we can either use existing VIRTIO_NET_HDR_GSO_UDP on receive
> path, add some kind of negotiation logic to packet socket (like
> storing the application expected vnet header size), or perform
> IPv6 fragmentation somehow.
>
> Options 1 or 2 are doable.
1 is using VIRTIO_NET_HDR_GSO_UDP and discarding ID,
2 is "some kind of negotiation logic"?
2 can't be enough, you will need 1 as well.
So let's start with 1 as a first step.
> >
> > 2. Userspace doesn't have a way to set fragment IDs
> > for tun/macvtap/packet sockets.
> > Presumably, not all users have these IDs. E.g. old
> > guests clearly don't.
> >
> > We should either generate our own ID,
> > like we always did, or make sure we don't accept
> > these packets.
> > Second option is almost sure to break userspace,
> > so it seems we must do the first one.
> >
>
> Right. This was missing from packet sockets. I can fix it.
>
> -vlad
Also, this can't be a patch on top, since we don't
want bisect to give us configurations which
can BUG().
> >
> > 3.
> > Exactly the same two things if you replace userspace
> > with hypervisor and tun/macvtap/packet socket with
> > virtio device.
> >
> >
> > 4.
> > As a next step, we should add a way for userspace
> > to tell us that it has ids and can handle them.
> >
> >
> >
> >
> >
> >>>
> >>>> diff --git a/net/core/dev.c b/net/core/dev.c
> >>>> index 945bbd0..fa4d2ee 100644
> >>>> --- a/net/core/dev.c
> >>>> +++ b/net/core/dev.c
> >>>> @@ -5929,6 +5929,12 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
> >>>> features &= ~NETIF_F_ALL_TSO;
> >>>> }
> >>>>
> >>>> + /* UFO requires that SG is present as well */
> >>>> + if ((features & NETIF_F_ALL_UFO) && !(features & NETIF_F_SG)) {
> >>>> + netdev_dbg(dev, "Dropping UFO features since no SG feature.\n");
> >>>> + features &= ~NETIF_F_ALL_UFO;
> >>>> + }
> >>>> +
> >>>> if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
> >>>> !(features & NETIF_F_IP_CSUM)) {
> >>>> netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
> >>>> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
> >>>> features &= ~NETIF_F_GSO;
> >>>> }
> >>>>
> >>>> - /* UFO needs SG and checksumming */
> >>>> - if (features & NETIF_F_UFO) {
> >>>> - /* maybe split UFO into V4 and V6? */
> >>>> - if (!((features & NETIF_F_GEN_CSUM) ||
> >>>> - (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> >>>> - == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> >>>> - netdev_dbg(dev,
> >>>> - "Dropping NETIF_F_UFO since no checksum offload features.\n");
> >>>> - features &= ~NETIF_F_UFO;
> >>>> - }
> >>>> -
> >>>> - if (!(features & NETIF_F_SG)) {
> >>>> - netdev_dbg(dev,
> >>>> - "Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
> >>>> - features &= ~NETIF_F_UFO;
> >>>> - }
> >>>> + /* UFO also needs checksumming */
> >>>> + if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
> >>>> + !(features & NETIF_F_IP_CSUM)) {
> >>>> + netdev_dbg(dev,
> >>>> + "Dropping NETIF_F_UFO since no checksum offload features.\n");
> >>>> + features &= ~NETIF_F_UFO;
> >>>> + }
> >>>> + if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
> >>>> + !(features & NETIF_F_IPV6_CSUM)) {
> >>>> + netdev_dbg(dev,
> >>>> + "Dropping NETIF_F_UFO6 since no checksum offload features.\n");
> >>>> + features &= ~NETIF_F_UFO6;
> >>>> }
> >>>>
> >>>> +
> >>>> #ifdef CONFIG_NET_RX_BUSY_POLL
> >>>> if (dev->netdev_ops->ndo_busy_poll)
> >>>> features |= NETIF_F_BUSY_POLL;
> >>>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> >>>> index 06dfb29..93eff41 100644
> >>>> --- a/net/core/ethtool.c
> >>>> +++ b/net/core/ethtool.c
> >>>> @@ -223,7 +223,7 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
> >>>> return NETIF_F_ALL_TSO;
> >>>> case ETHTOOL_GUFO:
> >>>> case ETHTOOL_SUFO:
> >>>> - return NETIF_F_UFO;
> >>>> + return NETIF_F_ALL_UFO;
> >>>> case ETHTOOL_GGSO:
> >>>> case ETHTOOL_SGSO:
> >>>> return NETIF_F_GSO;
> >>>> --
> >>>> 1.9.3
^ permalink raw reply
* Re: [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks
From: David Miller @ 2014-12-18 17:25 UTC (permalink / raw)
To: elfring
Cc: sergei.shtylyov, paulus, linux-ppp, netdev, eric.dumazet,
linux-kernel, kernel-janitors, julia.lawall
In-Reply-To: <54930D7C.3000901@users.sourceforge.net>
From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Dec 2014 18:23:08 +0100
>>> Where should "the error pointers" be stored instead?
>>
>> A local variable, before you assign it into the datastructure.
>
> I have looked at the affected software infrastructure once more.
> Now I find still that your data reorgansisation wish can not be resolved
> in a simple way.
I'm saying to leave the code alone.
If it goes:
var = foo_that_returns_ptr_err()
if (IS_ERR(var))
return PTR_ERR(var);
p->bar = var;
or whatever, simply keep it that way!
I'm not engaging in this conversation any further, you have already
consumed way too much of my limited time on this incredibly trivial
matter.
^ permalink raw reply
* RE: [RFC PATCH net-next v2 1/1] net: Support for switch port configuration
From: Varlese, Marco @ 2014-12-18 17:25 UTC (permalink / raw)
To: Roopa Prabhu
Cc: netdev@vger.kernel.org, Fastabend, John R, Thomas Graf,
Jiri Pirko, sfeldma@gmail.com, linux-kernel@vger.kernel.org
In-Reply-To: <5492EFC3.8030102@cumulusnetworks.com>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Roopa Prabhu
> Sent: Thursday, December 18, 2014 3:16 PM
> To: Varlese, Marco
> Cc: netdev@vger.kernel.org; Fastabend, John R; Thomas Graf; Jiri Pirko;
> sfeldma@gmail.com; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH net-next v2 1/1] net: Support for switch port
> configuration
>
> On 12/18/14, 6:55 AM, Varlese, Marco wrote:
> >> -----Original Message-----
> >> From: Roopa Prabhu [mailto:roopa@cumulusnetworks.com]
> >> Sent: Thursday, December 18, 2014 2:45 PM
> >> To: Varlese, Marco
> >> Cc: netdev@vger.kernel.org; Fastabend, John R; Thomas Graf; Jiri
> >> Pirko; sfeldma@gmail.com; linux-kernel@vger.kernel.org
> >> Subject: Re: [RFC PATCH net-next v2 1/1] net: Support for switch port
> >> configuration
> >>
> >> On 12/18/14, 3:29 AM, Varlese, Marco wrote:
> >>> From: Marco Varlese <marco.varlese@intel.com>
> >>>
> >>> Switch hardware offers a list of attributes that are configurable on
> >>> a per port basis.
> >>> This patch provides a mechanism to configure switch ports by adding
> >>> an NDO for setting specific values to specific attributes.
> >>> There will be a separate patch that adds the "get" functionality via
> >>> another NDO and another patch that extends iproute2 to call the two
> >>> new NDOs.
> >>>
> >>> Signed-off-by: Marco Varlese <marco.varlese@intel.com>
> >>> ---
> >>> include/linux/netdevice.h | 5 ++++
> >>> include/uapi/linux/if_link.h | 15 ++++++++++++
> >>> net/core/rtnetlink.c | 54
> >> ++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 74 insertions(+)
> >>>
> >>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >>> index c31f74d..4881c7b 100644
> >>> --- a/include/linux/netdevice.h
> >>> +++ b/include/linux/netdevice.h
> >>> @@ -1027,6 +1027,9 @@ typedef u16 (*select_queue_fallback_t)(struct
> >> net_device *dev,
> >>> * int (*ndo_switch_port_stp_update)(struct net_device *dev, u8
> state);
> >>> * Called to notify switch device port of bridge port STP
> >>> * state change.
> >>> + * int (*ndo_switch_port_set_cfg)(struct net_device *dev,
> >>> + * u32 attr, u64 value);
> >>> + * Called to set specific switch ports attributes.
> >>> */
> >>> struct net_device_ops {
> >>> int (*ndo_init)(struct net_device *dev);
> >>> @@ -1185,6 +1188,8 @@ struct net_device_ops {
> >>> struct
> >> netdev_phys_item_id *psid);
> >>> int (*ndo_switch_port_stp_update)(struct
> >> net_device *dev,
> >>> u8 state);
> >>> + int (*ndo_switch_port_set_cfg)(struct
> >> net_device *dev,
> >>> + u32 attr, u64 value);
> >>> #endif
> >>> };
> >>>
> >>> diff --git a/include/uapi/linux/if_link.h
> >>> b/include/uapi/linux/if_link.h index f7d0d2d..19cb51a 100644
> >>> --- a/include/uapi/linux/if_link.h
> >>> +++ b/include/uapi/linux/if_link.h
> >>> @@ -146,6 +146,7 @@ enum {
> >>> IFLA_PHYS_PORT_ID,
> >>> IFLA_CARRIER_CHANGES,
> >>> IFLA_PHYS_SWITCH_ID,
> >>> + IFLA_SWITCH_PORT_CFG,
> >>> __IFLA_MAX
> >>> };
> >>>
> >>> @@ -603,4 +604,18 @@ enum {
> >>>
> >>> #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
> >>>
> >>> +/* Switch Port Attributes section */
> >>> +
> >>> +enum {
> >>> + IFLA_ATTR_UNSPEC,
> >>> + IFLA_ATTR_LEARNING,
> >> Any reason you want learning here ?. This is covered as part of the
> >> bridge setlink attributes.
> >>
> > Yes, because the user may _not_ want to go through a bridge interface
> necessarily.
> But, the bridge setlink/getlink interface was changed to accommodate 'self'
> for exactly such cases.
> I kind of understand your case for the other attributes (these are per port
> settings that switch asics provide).
>
> However, i don't understand the reason to pull in bridge attributes here.
>
Maybe, I am missing something so you might help. The learning attribute - in my case - it is like all other attributes: a port attribute (as you said, port settings that the switch provides per port).
So, what I was saying is "why the user shall go through a bridge to configure the learning attribute"? From my perspective, it is as any other attribute and as such configurable on the port.
> >
> >>> + IFLA_ATTR_LOOPBACK,
> >>> + IFLA_ATTR_BCAST_FLOODING,
> >>> + IFLA_ATTR_UCAST_FLOODING,
> >>> + IFLA_ATTR_MCAST_FLOODING,
> >>> + __IFLA_ATTR_MAX
> >>> +};
> >>> +
> >>> +#define IFLA_ATTR_MAX (__IFLA_ATTR_MAX - 1)
> >>> +
> >>> #endif /* _UAPI_LINUX_IF_LINK_H */ diff --git
> >>> a/net/core/rtnetlink.c b/net/core/rtnetlink.c index
> >>> eaa057f..c0d1c45 100644
> >>> --- a/net/core/rtnetlink.c
> >>> +++ b/net/core/rtnetlink.c
> >>> @@ -1389,6 +1389,46 @@ static int validate_linkmsg(struct net_device
> >> *dev, struct nlattr *tb[])
> >>> return 0;
> >>> }
> >>>
> >>> +#ifdef CONFIG_NET_SWITCHDEV
> >>> +static int do_setswcfg(struct net_device *dev, struct nlattr *attr) {
> >>> + int rem, err = -EINVAL;
> >>> + struct nlattr *v;
> >>> + const struct net_device_ops *ops = dev->netdev_ops;
> >>> +
> >>> + nla_for_each_nested(v, attr, rem) {
> >>> + u32 op = nla_type(v);
> >>> + u64 value = 0;
> >>> +
> >>> + switch (op) {
> >>> + case IFLA_ATTR_LEARNING:
> >>> + case IFLA_ATTR_LOOPBACK:
> >>> + case IFLA_ATTR_BCAST_FLOODING:
> >>> + case IFLA_ATTR_UCAST_FLOODING:
> >>> + case IFLA_ATTR_MCAST_FLOODING: {
> >>> + if (nla_len(v) < sizeof(value)) {
> >>> + err = -EINVAL;
> >>> + break;
> >>> + }
> >>> +
> >>> + value = nla_get_u64(v);
> >>> + err = ops->ndo_switch_port_set_cfg(dev,
> >>> + op,
> >>> + value);
> >>> + break;
> >>> + }
> >>> + default:
> >>> + err = -EINVAL;
> >>> + break;
> >>> + }
> >>> + if (err)
> >>> + break;
> >>> + }
> >>> + return err;
> >>> +}
> >>> +
> >>> +#endif
> >>> +
> >>> static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
> >>> {
> >>> int rem, err = -EINVAL;
> >>> @@ -1740,6 +1780,20 @@ static int do_setlink(const struct sk_buff *skb,
> >>> status |= DO_SETLINK_NOTIFY;
> >>> }
> >>> }
> >>> +#ifdef CONFIG_NET_SWITCHDEV
> >>> + if (tb[IFLA_SWITCH_PORT_CFG]) {
> >>> + err = -EOPNOTSUPP;
> >>> + if (!ops->ndo_switch_port_set_cfg)
> >>> + goto errout;
> >>> + if (!ops->ndo_switch_parent_id_get)
> >>> + goto errout;
> >>> + err = do_setswcfg(dev, tb[IFLA_SWITCH_PORT_CFG]);
> >>> + if (err < 0)
> >>> + goto errout;
> >>> +
> >>> + status |= DO_SETLINK_NOTIFY;
> >>> + }
> >>> +#endif
> >>> err = 0;
> >>>
> >>> errout:
> > --
> > 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
>
> --
> 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 v2 0/6] net-PPP: Deletion of a few unnecessary checks
From: SF Markus Elfring @ 2014-12-18 17:23 UTC (permalink / raw)
To: David Miller
Cc: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet,
linux-kernel, kernel-janitors, Julia Lawall
In-Reply-To: <20141212.115922.687789059853236747.davem@davemloft.net>
>> Where should "the error pointers" be stored instead?
>
> A local variable, before you assign it into the datastructure.
I have looked at the affected software infrastructure once more.
Now I find still that your data reorgansisation wish can not be resolved
in a simple way.
I imagine that your update suggestion would mean that the corresponding
pointers will be passed around by function parameters instead,
wouldn't it?
Two pointers were stored as the members "arc4" and "sha1" of the
data structure "ppp_mppe_state" for a specific reason. A pointer to
this structure is passed to the ppp_register_compressor() function.
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/include/linux/ppp-comp.h?id=607ca46e97a1b6594b29647d98a32d545c24bdff#n32
The data structure "compressor" manages some function pointers.
I assume that this interface should not be changed at the moment, should it?
Are further ideas needed here?
Regards,
Markus
^ permalink raw reply
* RE: [RFC PATCH net-next v2 1/1] net: Support for switch port configuration
From: Arad, Ronen @ 2014-12-18 17:17 UTC (permalink / raw)
To: Fastabend, John R, Varlese, Marco, Roopa Prabhu,
netdev@vger.kernel.org
Cc: Thomas Graf, Jiri Pirko, sfeldma@gmail.com,
linux-kernel@vger.kernel.org
In-Reply-To: <5492FD51.5030503@intel.com>
> -----Original Message-----
> From: Fastabend, John R
> Sent: Thursday, December 18, 2014 6:14 PM
> To: Arad, Ronen; Varlese, Marco; Roopa Prabhu; netdev@vger.kernel.org
> Cc: Thomas Graf; Jiri Pirko; sfeldma@gmail.com; linux-
> kernel@vger.kernel.org
> Subject: Re: [RFC PATCH net-next v2 1/1] net: Support for switch port
> configuration
>
> On 12/18/2014 07:47 AM, Arad, Ronen wrote:
> >
> >
> >> -----Original Message-----
> >> From: netdev-owner@vger.kernel.org [mailto:netdev-
> >> owner@vger.kernel.org] On Behalf Of Varlese, Marco
> >> Sent: Thursday, December 18, 2014 4:56 PM
> >> To: Roopa Prabhu
> >> Cc: netdev@vger.kernel.org; Fastabend, John R; Thomas Graf; Jiri
> >> Pirko; sfeldma@gmail.com; linux-kernel@vger.kernel.org
> >> Subject: RE: [RFC PATCH net-next v2 1/1] net: Support for switch port
> >> configuration
> >>
> >>> -----Original Message-----
> >>> From: Roopa Prabhu [mailto:roopa@cumulusnetworks.com]
> >>> Sent: Thursday, December 18, 2014 2:45 PM
> >>> To: Varlese, Marco
> >>> Cc: netdev@vger.kernel.org; Fastabend, John R; Thomas Graf; Jiri
> >>> Pirko; sfeldma@gmail.com; linux-kernel@vger.kernel.org
> >>> Subject: Re: [RFC PATCH net-next v2 1/1] net: Support for switch
> >>> port configuration
> >>>
> >>> On 12/18/14, 3:29 AM, Varlese, Marco wrote:
> >>>> From: Marco Varlese <marco.varlese@intel.com>
> >>>>
> >>>> Switch hardware offers a list of attributes that are configurable
> >>>> on a per port basis.
> >>>> This patch provides a mechanism to configure switch ports by adding
> >>>> an NDO for setting specific values to specific attributes.
> >>>> There will be a separate patch that adds the "get" functionality
> >>>> via another NDO and another patch that extends iproute2 to call the
> >>>> two new NDOs.
> >>>>
> >>>> Signed-off-by: Marco Varlese <marco.varlese@intel.com>
> >>>> ---
> >>>> include/linux/netdevice.h | 5 ++++
> >>>> include/uapi/linux/if_link.h | 15 ++++++++++++
> >>>> net/core/rtnetlink.c | 54
> >>> ++++++++++++++++++++++++++++++++++++++++++++
> >>>> 3 files changed, 74 insertions(+)
> >>>>
> >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >>>> index c31f74d..4881c7b 100644
> >>>> --- a/include/linux/netdevice.h
> >>>> +++ b/include/linux/netdevice.h
> >>>> @@ -1027,6 +1027,9 @@ typedef u16
> (*select_queue_fallback_t)(struct
> >>> net_device *dev,
> >>>> * int (*ndo_switch_port_stp_update)(struct net_device *dev, u8
> state);
> >>>> * Called to notify switch device port of bridge port STP
> >>>> * state change.
> >>>> + * int (*ndo_switch_port_set_cfg)(struct net_device *dev,
> >>>> + * u32 attr, u64 value);
> >>>> + * Called to set specific switch ports attributes.
> >>>> */
> >>>> struct net_device_ops {
> >>>> int (*ndo_init)(struct net_device *dev);
> >>>> @@ -1185,6 +1188,8 @@ struct net_device_ops {
> >>>> struct
> >>> netdev_phys_item_id *psid);
> >>>> int (*ndo_switch_port_stp_update)(struct
> >>> net_device *dev,
> >>>> u8 state);
> >>>> + int (*ndo_switch_port_set_cfg)(struct
> >>> net_device *dev,
> >>>> + u32 attr, u64 value);
> >>>> #endif
> >>>> };
> >>>>
> >>>> diff --git a/include/uapi/linux/if_link.h
> >>>> b/include/uapi/linux/if_link.h index f7d0d2d..19cb51a 100644
> >>>> --- a/include/uapi/linux/if_link.h
> >>>> +++ b/include/uapi/linux/if_link.h
> >>>> @@ -146,6 +146,7 @@ enum {
> >>>> IFLA_PHYS_PORT_ID,
> >>>> IFLA_CARRIER_CHANGES,
> >>>> IFLA_PHYS_SWITCH_ID,
> >>>> + IFLA_SWITCH_PORT_CFG,
> >>>> __IFLA_MAX
> >>>> };
> >>>>
> >>>> @@ -603,4 +604,18 @@ enum {
> >>>>
> >>>> #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
> >>>>
> >>>> +/* Switch Port Attributes section */
> >>>> +
> >>>> +enum {
> >>>> + IFLA_ATTR_UNSPEC,
> >>>> + IFLA_ATTR_LEARNING,
> >>> Any reason you want learning here ?. This is covered as part of the
> >>> bridge setlink attributes.
> >>>
> >>
> >> Yes, because the user may _not_ want to go through a bridge interface
> >> necessarily.
> >
> > Some bridge attributes or more accurately bridge port attributes overlap
> with port attributes.
> > IFLA_BRPORT_LEARNING from IFLA_PROTINFO and
> BRIDGE_VLAN_INFO_PVID flag
> > of IFLA_BRIDGE_VLAN_INFO from IFLA_AF_SPEC are two examples where
> > bridge port properties might have to be mapped to a common or driver
> > specific port attribute.
>
> You've lost me a bit.
>
> > Switch port driver that works without and explicit bridge device has
> > to implement the bridge NDO's ndo_bridge_setlink/ndo_bridge_dellink.
> > It could take care of mapping such attributes from the bridge netlink
> > message to either driver-specific port attribute of to an explicit one
> > (assuming IFLA_ATTR_LEARNING would be accepted). For example the
> > driver could process the bridge learning information from the protinfo
> > as such:
>
> still lost unfortunately.
>
> So you want to enable learning on a port by port basis? Then we also need to
> define how the two bits interact.
>
> switch_learning + port_learning = port in learning mode
> !switch_learning + port_learning = port not in learning mode?
> etc.
>
This is a valid issue. I don't believe there is a way for the switch port driver to figure out whether an AF_BRIDGE RTM_SETLINK message was intended to the bridge or to the specific port targeted. It could only make such decision separately for each attribute configured.
When an explicit bridge instance is present, directing the netlink message to the bridge with SELF flag implies bridge property; Directing it to a bridge port netdev with MASTER implies bridge port property.
> Do bridges and users actually enable learning on a per port basis?
My understanding is that learning configuration is already on a port by port basis. That's what I see in the rocker driver. The LERNING and LEARNING_SYNC are stored in the brport_flags of a rocker_port structure.
My statement was that synchronization is needed between bridge port configuration and equivalent switch port attribute.
The switch port driver implementing bridge setlink NDOs has to take care of such synchronization.
>
> > int sw_driver_br_setlink(struct net_device *dev, struct nlmsghdr *nlh)
> > {
> > struct nlattr *protinfo;
> > protinfo = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg),
> > IFLA_PROTINFO);
> > if (protinfo) {
> > struct nlattr *attr;
> > attr = nla_find_nested(protinfo, IFLA_BRPORT_LEARNING);
> > if (attr) {
> > if (nla_len(attr) >= sizeof(u8)) {
> > if (nla_get_u8(attr))
> > brport_flags |= BR_LEARNING;
> > else
> > brport_flags &= ~BR_LEARNING;
> > }
> > }
> > /*
> > * Map bridge port attributes to driver-specific port attributes
> > */
> > }
> > return 0;
> > }
> >
> >>
> >>>> + IFLA_ATTR_LOOPBACK,
> >>>> + IFLA_ATTR_BCAST_FLOODING,
> >>>> + IFLA_ATTR_UCAST_FLOODING,
> >>>> + IFLA_ATTR_MCAST_FLOODING,
> >>>> + __IFLA_ATTR_MAX
> >>>> +};
> >>>> +
> >>>> +#define IFLA_ATTR_MAX (__IFLA_ATTR_MAX - 1)
> >>>> +
> >>>> #endif /* _UAPI_LINUX_IF_LINK_H */ diff --git
> >>>> a/net/core/rtnetlink.c b/net/core/rtnetlink.c index
> >>>> eaa057f..c0d1c45 100644
> >>>> --- a/net/core/rtnetlink.c
> >>>> +++ b/net/core/rtnetlink.c
> >>>> @@ -1389,6 +1389,46 @@ static int validate_linkmsg(struct
> >>>> net_device
> >>> *dev, struct nlattr *tb[])
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +#ifdef CONFIG_NET_SWITCHDEV
> >>>> +static int do_setswcfg(struct net_device *dev, struct nlattr *attr) {
> >>>> + int rem, err = -EINVAL;
> >>>> + struct nlattr *v;
> >>>> + const struct net_device_ops *ops = dev->netdev_ops;
> >>>> +
> >>>> + nla_for_each_nested(v, attr, rem) {
> >>>> + u32 op = nla_type(v);
> >>>> + u64 value = 0;
> >>>> +
> >>>> + switch (op) {
> >>>> + case IFLA_ATTR_LEARNING:
> >>>> + case IFLA_ATTR_LOOPBACK:
> >>>> + case IFLA_ATTR_BCAST_FLOODING:
> >>>> + case IFLA_ATTR_UCAST_FLOODING:
> >>>> + case IFLA_ATTR_MCAST_FLOODING: {
> >>>> + if (nla_len(v) < sizeof(value)) {
> >>>> + err = -EINVAL;
> >>>> + break;
> >>>> + }
> >>>> +
> >>>> + value = nla_get_u64(v);
> >>>> + err = ops->ndo_switch_port_set_cfg(dev,
> >>>> + op,
> >>>> + value);
> >>>> + break;
> >>>> + }
> >>>> + default:
> >>>> + err = -EINVAL;
> >>>> + break;
> >>>> + }
> >>>> + if (err)
> >>>> + break;
> >>>> + }
> >>>> + return err;
> >>>> +}
> >>>> +
> >>>> +#endif
> >>>> +
> >>>> static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
> >>>> {
> >>>> int rem, err = -EINVAL;
> >>>> @@ -1740,6 +1780,20 @@ static int do_setlink(const struct sk_buff
> *skb,
> >>>> status |= DO_SETLINK_NOTIFY;
> >>>> }
> >>>> }
> >>>> +#ifdef CONFIG_NET_SWITCHDEV
> >>>> + if (tb[IFLA_SWITCH_PORT_CFG]) {
> >>>> + err = -EOPNOTSUPP;
> >>>> + if (!ops->ndo_switch_port_set_cfg)
> >>>> + goto errout;
> >>>> + if (!ops->ndo_switch_parent_id_get)
> >>>> + goto errout;
> >>>> + err = do_setswcfg(dev, tb[IFLA_SWITCH_PORT_CFG]);
> >>>> + if (err < 0)
> >>>> + goto errout;
> >>>> +
> >>>> + status |= DO_SETLINK_NOTIFY;
> >>>> + }
> >>>> +#endif
> >>>> err = 0;
> >>>>
> >>>> errout:
> >>
> >> --
> >> 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
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