* Re: [PATCH 0/3] net: ethernet: atheros: atlx: Use PCI generic definitions instead of private duplicates
From: Chris Snook @ 2019-06-21 22:45 UTC (permalink / raw)
To: Joe Perches
Cc: Bjorn Helgaas, Puranjay Mohan, Jay Cliburn, Shuah Khan,
Bjorn Helgaas, netdev, Linux Kernel Mailing List,
linux-kernel-mentees, Linux PCI
In-Reply-To: <838b8e84523151418ab8cda4abdbb114ce24a497.camel@perches.com>
On Fri, Jun 21, 2019 at 11:33 AM Joe Perches <joe@perches.com> wrote:
>
> On Fri, 2019-06-21 at 13:12 -0500, Bjorn Helgaas wrote:
> > On Fri, Jun 21, 2019 at 12:27 PM Joe Perches <joe@perches.com> wrote:
> []
> > > Subsystem specific local PCI #defines without generic
> > > naming is poor style and makes treewide grep and
> > > refactoring much more difficult.
> >
> > Don't worry, we have the same objectives. I totally agree that local
> > #defines are a bad thing, which is why I proposed this project in the
> > first place.
>
> Hi again Bjorn.
>
> I didn't know that was your idea. Good idea.
>
> > I'm just saying that this is a "first-patch" sort of learning project
> > and I think it'll avoid some list spamming and discouragement if we
> > can figure out the scope and shake out some of the teething problems
> > ahead of time. I don't want to end up with multiple versions of
> > dozens of little 2-3 patch series posted every week or two.
>
> Great, that's sensible.
>
> > I'd rather be able to deal with a whole block of them at one time.
>
> Also very sensible.
>
> > > 2: Show that you compiled the object files and verified
> > > where possible that there are no object file changes.
> >
> > Do you have any pointers for the best way to do this? Is it as simple
> > as comparing output of "objdump -d"?
>
> Generically, yes.
>
> I have a little script that does the equivalent of:
>
> <git reset>
> make <foo.o>
> mv <foo.o> <foo.o>.old
> patch -P1 < <foo_patch>
> make <foo.o>
> mv <foo.o> <foo.o>.new
> diff -urN <(objdump -d <foo.o>.old) <(objdump -d <foo.o>.new)
>
> But it's not foolproof as gcc does not guarantee
> compilation repeatability.
>
> And some subsystems Makefiles do not allow per-file
> compilation.
>
This should work, but be aware that the older atlx drivers did some
regrettable things with file structure, so not all .c files are
expected to generate a corresponding .o file.
- Chris
^ permalink raw reply
* [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Takshak Chahande @ 2019-06-21 22:33 UTC (permalink / raw)
To: netdev; +Cc: ast, daniel, rdna, ctakshak, kernel-team
With different bpf attach_flags available to attach bpf programs specially
with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective
bpf-programs available to any sub-cgroups really needs to be available for
easy debugging.
Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached
bpf-programs to a cgroup but also the inherited ones from parent cgroup.
So "-e" option is introduced to use BPF_F_QUERY_EFFECTIVE query flag here to
list all the effective bpf-programs available for execution at a specified
cgroup.
Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf:
# ./test_cgroup_attach
With old bpftool (without -e option):
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
ID AttachType AttachFlags Name
271 egress multi pkt_cntr_1
272 egress multi pkt_cntr_2
Attached new program pkt_cntr_4 in cg2 gives following:
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2
ID AttachType AttachFlags Name
273 egress override pkt_cntr_4
And with new "-e" option it shows all effective programs for cg2:
# bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2
ID AttachType AttachFlags Name
273 egress override pkt_cntr_4
271 egress override pkt_cntr_1
272 egress override pkt_cntr_2
Signed-off-by: Takshak Chahande <ctakshak@fb.com>
Acked-by: Andrey Ignatov <rdna@fb.com>
---
tools/bpf/bpftool/Documentation/bpftool-cgroup.rst | 8 +++++++-
tools/bpf/bpftool/Documentation/bpftool.rst | 6 +++++-
tools/bpf/bpftool/bash-completion/bpftool | 2 +-
tools/bpf/bpftool/cgroup.c | 7 ++++---
tools/bpf/bpftool/main.c | 7 ++++++-
tools/bpf/bpftool/main.h | 3 ++-
6 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index 36807735e2a5..5e515aac36b3 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -12,7 +12,8 @@ SYNOPSIS
**bpftool** [*OPTIONS*] **cgroup** *COMMAND*
- *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
+ *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** }
+ | { **-e** | **--effective** } }
*COMMANDS* :=
{ **show** | **list** | **tree** | **attach** | **detach** | **help** }
@@ -117,6 +118,11 @@ OPTIONS
Print all logs available from libbpf, including debug-level
information.
+ -e, --effective
+ Retrieve effective programs that will execute for events
+ within a cgroup. This includes inherited along with attached
+ ones.
+
EXAMPLES
========
|
diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index 6a9c52ef84a9..d2f76b55988d 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -19,7 +19,7 @@ SYNOPSIS
*OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** }
*OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** }
- | { **-j** | **--json** } [{ **-p** | **--pretty** }] }
+ | { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } }
*MAP-COMMANDS* :=
{ **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext**
@@ -71,6 +71,10 @@ OPTIONS
includes logs from libbpf as well as from the verifier, when
attempting to load programs.
+ -e, --effective
+ Retrieve effective programs that will execute for events
+ within a cgroup. This includes inherited along with attached ones.
+
SEE ALSO
========
**bpf**\ (2),
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 2725e27dfa42..72fd832072a3 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -187,7 +187,7 @@ _bpftool()
# Deal with options
if [[ ${words[cword]} == -* ]]; then
- local c='--version --json --pretty --bpffs --mapcompat --debug'
+ local c='--version --json --pretty --bpffs --mapcompat --debug --effective'
COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
return 0
fi
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
index 7e22f115c8c1..86f9ac8c4599 100644
--- a/tools/bpf/bpftool/cgroup.c
+++ b/tools/bpf/bpftool/cgroup.c
@@ -101,7 +101,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
__u32 prog_cnt = 0;
int ret;
- ret = bpf_prog_query(cgroup_fd, type, 0, NULL, NULL, &prog_cnt);
+ ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
+ &prog_cnt);
if (ret)
return -1;
@@ -119,8 +120,8 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
int ret;
prog_cnt = ARRAY_SIZE(prog_ids);
- ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids,
- &prog_cnt);
+ ret = bpf_prog_query(cgroup_fd, type, query_flags, &attach_flags,
+ prog_ids, &prog_cnt);
if (ret)
return ret;
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 4879f6395c7e..42e9ddfbbbe0 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -27,6 +27,7 @@ bool json_output;
bool show_pinned;
bool block_mount;
bool verifier_logs;
+unsigned int query_flags;
int bpf_flags;
struct pinned_obj_table prog_table;
struct pinned_obj_table map_table;
@@ -327,6 +328,7 @@ int main(int argc, char **argv)
{ "mapcompat", no_argument, NULL, 'm' },
{ "nomount", no_argument, NULL, 'n' },
{ "debug", no_argument, NULL, 'd' },
+ { "effective", no_argument, NULL, 'e' },
{ 0 }
};
int opt, ret;
@@ -342,7 +344,7 @@ int main(int argc, char **argv)
hash_init(map_table.table);
opterr = 0;
- while ((opt = getopt_long(argc, argv, "Vhpjfmnd",
+ while ((opt = getopt_long(argc, argv, "Vhpjfmnde",
options, NULL)) >= 0) {
switch (opt) {
case 'V':
@@ -376,6 +378,9 @@ int main(int argc, char **argv)
libbpf_set_print(print_all_levels);
verifier_logs = true;
break;
+ case 'e':
+ query_flags = BPF_F_QUERY_EFFECTIVE;
+ break;
default:
p_err("unrecognized option '%s'", argv[optind - 1]);
if (json_output)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 28a2a5857e14..fddec15c454a 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -45,7 +45,7 @@
"PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }"
#define HELP_SPEC_OPTIONS \
"OPTIONS := { {-j|--json} [{-p|--pretty}] | {-f|--bpffs} |\n" \
- "\t {-m|--mapcompat} | {-n|--nomount} }"
+ "\t {-m|--mapcompat} | {-n|--nomount} | {-e|--effective} }"
#define HELP_SPEC_MAP \
"MAP := { id MAP_ID | pinned FILE }"
@@ -92,6 +92,7 @@ extern bool json_output;
extern bool show_pinned;
extern bool block_mount;
extern bool verifier_logs;
+extern unsigned int query_flags;
extern int bpf_flags;
extern struct pinned_obj_table prog_table;
extern struct pinned_obj_table map_table;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next 13/18] ionic: Add initial ethtool support
From: Shannon Nelson @ 2019-06-21 22:30 UTC (permalink / raw)
To: Michal Kubecek, netdev
In-Reply-To: <20190621023205.GD21796@unicorn.suse.cz>
On 6/20/19 7:32 PM, Michal Kubecek wrote:
> On Thu, Jun 20, 2019 at 01:24:19PM -0700, Shannon Nelson wrote:
>> Add in the basic ethtool callbacks for device information
>> and control.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
> ...
>> +static int ionic_get_link_ksettings(struct net_device *netdev,
>> + struct ethtool_link_ksettings *ks)
>> +{
>> + struct lif *lif = netdev_priv(netdev);
>> + struct ionic_dev *idev = &lif->ionic->idev;
>> + int copper_seen = 0;
>> +
>> + ethtool_link_ksettings_zero_link_mode(ks, supported);
>> + ethtool_link_ksettings_zero_link_mode(ks, advertising);
>> +
>> + if (ionic_is_mnic(lif->ionic)) {
>> + ethtool_link_ksettings_add_link_mode(ks, supported, Backplane);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising, Backplane);
>> + } else {
>> + ethtool_link_ksettings_add_link_mode(ks, supported, FIBRE);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising, FIBRE);
>> +
>> + if (ionic_is_pf(lif->ionic)) {
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + Autoneg);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + Autoneg);
>> + }
>> + }
>> +
>> + switch (le16_to_cpu(idev->port_info->status.xcvr.pid)) {
>> + /* Copper */
>> + case XCVR_PID_QSFP_100G_CR4:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 100000baseCR4_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 100000baseCR4_Full);
>> + copper_seen++;
>> + break;
>> + case XCVR_PID_QSFP_40GBASE_CR4:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 40000baseCR4_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 40000baseCR4_Full);
>> + copper_seen++;
>> + break;
>> + case XCVR_PID_SFP_25GBASE_CR_S:
>> + case XCVR_PID_SFP_25GBASE_CR_L:
>> + case XCVR_PID_SFP_25GBASE_CR_N:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 25000baseCR_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 25000baseCR_Full);
>> + copper_seen++;
>> + break;
>> + case XCVR_PID_SFP_10GBASE_AOC:
>> + case XCVR_PID_SFP_10GBASE_CU:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 10000baseCR_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 10000baseCR_Full);
>> + copper_seen++;
>> + break;
>> +
>> + /* Fibre */
>> + case XCVR_PID_QSFP_100G_SR4:
>> + case XCVR_PID_QSFP_100G_AOC:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 100000baseSR4_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 100000baseSR4_Full);
>> + break;
>> + case XCVR_PID_QSFP_100G_LR4:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 100000baseLR4_ER4_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 100000baseLR4_ER4_Full);
>> + break;
>> + case XCVR_PID_QSFP_100G_ER4:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 100000baseLR4_ER4_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 100000baseLR4_ER4_Full);
>> + break;
>> + case XCVR_PID_QSFP_40GBASE_SR4:
>> + case XCVR_PID_QSFP_40GBASE_AOC:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 40000baseSR4_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 40000baseSR4_Full);
>> + break;
>> + case XCVR_PID_QSFP_40GBASE_LR4:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 40000baseLR4_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 40000baseLR4_Full);
>> + break;
>> + case XCVR_PID_SFP_25GBASE_SR:
>> + case XCVR_PID_SFP_25GBASE_AOC:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 25000baseSR_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 25000baseSR_Full);
>> + break;
>> + case XCVR_PID_SFP_10GBASE_SR:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 10000baseSR_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 10000baseSR_Full);
>> + break;
>> + case XCVR_PID_SFP_10GBASE_LR:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 10000baseLR_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 10000baseLR_Full);
>> + break;
>> + case XCVR_PID_SFP_10GBASE_LRM:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 10000baseLRM_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 10000baseLRM_Full);
>> + break;
>> + case XCVR_PID_SFP_10GBASE_ER:
>> + ethtool_link_ksettings_add_link_mode(ks, supported,
>> + 10000baseER_Full);
>> + ethtool_link_ksettings_add_link_mode(ks, advertising,
>> + 10000baseER_Full);
>> + break;
>> + case XCVR_PID_QSFP_100G_ACC:
>> + case XCVR_PID_QSFP_40GBASE_ER4:
>> + case XCVR_PID_SFP_25GBASE_LR:
>> + case XCVR_PID_SFP_25GBASE_ER:
>> + dev_info(lif->ionic->dev, "no decode bits for xcvr type pid=%d / 0x%x\n",
>> + idev->port_info->status.xcvr.pid,
>> + idev->port_info->status.xcvr.pid);
>> + break;
> Maybe you should rather add these modes so that they can be shown and
> set.
Yeah, I was thinking about that. I'll look at adding them in a separate
patchset.
>
>> + case XCVR_PID_UNKNOWN:
>> + break;
>> + default:
>> + dev_info(lif->ionic->dev, "unknown xcvr type pid=%d / 0x%x\n",
>> + idev->port_info->status.xcvr.pid,
>> + idev->port_info->status.xcvr.pid);
>> + break;
>> + }
> Up to this point, you always set each bit in both supported and
> advertised modes. Thus you could set the modes in only one of the
> bitmaps and copy it to the other here.
Good idea - that would clean up some of this silliness.
>
>> +
>> + ethtool_link_ksettings_add_link_mode(ks, supported, Pause);
>> + if (idev->port_info->config.pause_type)
>> + ethtool_link_ksettings_add_link_mode(ks, advertising, Pause);
>> +
>> + if (idev->port_info->config.fec_type == PORT_FEC_TYPE_FC)
>> + ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_BASER);
>> + else if (idev->port_info->config.fec_type == PORT_FEC_TYPE_RS)
>> + ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_RS);
>> + else
>> + ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_NONE);
> Is it correct to set these FEC bits only in advertising and not in
> supported?
Hmmm... good catch.
>
>> +static int ionic_set_link_ksettings(struct net_device *netdev,
>> + const struct ethtool_link_ksettings *ks)
>> +{
>> + struct lif *lif = netdev_priv(netdev);
>> + struct ionic *ionic = lif->ionic;
>> + struct ionic_dev *idev = &lif->ionic->idev;
>> + u8 fec_type = PORT_FEC_TYPE_NONE;
>> + u32 req_rs, req_b;
>> + int err = 0;
>> +
>> + /* set autoneg */
>> + if (ks->base.autoneg != idev->port_info->config.an_enable) {
>> + idev->port_info->config.an_enable = ks->base.autoneg;
> IMHO you should only save the value if the command below succeeds,
> otherwise next time you will be comparing against the value which wasn't
> actually set.
Yes, thanks.
>
>> + mutex_lock(&ionic->dev_cmd_lock);
>> + ionic_dev_cmd_port_autoneg(idev, ks->base.autoneg);
>> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout);
>> + mutex_unlock(&ionic->dev_cmd_lock);
>> + if (err)
>> + return err;
>> + }
>> +
>> + /* set speed */
>> + if (ks->base.speed != le32_to_cpu(idev->port_info->config.speed)) {
>> + idev->port_info->config.speed = cpu_to_le32(ks->base.speed);
> Same here.
Sure.
>
>> + mutex_lock(&ionic->dev_cmd_lock);
>> + ionic_dev_cmd_port_speed(idev, ks->base.speed);
>> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout);
>> + mutex_unlock(&ionic->dev_cmd_lock);
>> + if (err)
>> + return err;
>> + }
>> +
>> + /* set FEC */
>> + req_rs = ethtool_link_ksettings_test_link_mode(ks, advertising, FEC_RS);
>> + req_b = ethtool_link_ksettings_test_link_mode(ks, advertising, FEC_BASER);
>> + if (req_rs && req_b) {
>> + netdev_info(netdev, "Only select one FEC mode at a time\n");
>> + return -EINVAL;
>> +
>> + } else if (req_b &&
>> + idev->port_info->config.fec_type != PORT_FEC_TYPE_FC) {
>> + fec_type = PORT_FEC_TYPE_FC;
>> + } else if (req_rs &&
>> + idev->port_info->config.fec_type != PORT_FEC_TYPE_RS) {
>> + fec_type = PORT_FEC_TYPE_RS;
>> + } else if (!(req_rs | req_b) &&
>> + idev->port_info->config.fec_type != PORT_FEC_TYPE_NONE) {
>> + fec_type = PORT_FEC_TYPE_NONE;
>> + }
> AFAICS if userspace requests a mode which is already set, you end up
> with fec_type = PORT_FEC_TYPE_NONE here. This doesn't seem right.
> I assume you would rather want to skip the setting below in such case.
I'll double check that - thanks.
>
>> +
>> + idev->port_info->config.fec_type = fec_type;
>> + mutex_lock(&ionic->dev_cmd_lock);
>> + ionic_dev_cmd_port_fec(idev, PORT_FEC_TYPE_NONE);
> Shouldn't the argument be fec_type here?
Ugh... yes.
>
>> + err = ionic_dev_cmd_wait(ionic, devcmd_timeout);
>> + mutex_unlock(&ionic->dev_cmd_lock);
>> + if (err)
>> + return err;
>> +
>> + return 0;
>> +}
> ...
>> +static int ionic_set_ringparam(struct net_device *netdev,
>> + struct ethtool_ringparam *ring)
>> +{
>> + struct lif *lif = netdev_priv(netdev);
>> + bool running;
>> + int i, j;
>> +
>> + if (ring->rx_mini_pending || ring->rx_jumbo_pending) {
>> + netdev_info(netdev, "Changing jumbo or mini descriptors not supported\n");
>> + return -EINVAL;
>> + }
>> +
>> + i = ring->tx_pending & (ring->tx_pending - 1);
>> + j = ring->rx_pending & (ring->rx_pending - 1);
>> + if (i || j) {
>> + netdev_info(netdev, "Descriptor count must be a power of 2\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (ring->tx_pending > IONIC_MAX_TXRX_DESC ||
>> + ring->tx_pending < IONIC_MIN_TXRX_DESC ||
>> + ring->rx_pending > IONIC_MAX_TXRX_DESC ||
>> + ring->rx_pending < IONIC_MIN_TXRX_DESC) {
>> + netdev_info(netdev, "Descriptors count must be in the range [%d-%d]\n",
>> + IONIC_MIN_TXRX_DESC, IONIC_MAX_TXRX_DESC);
>> + return -EINVAL;
>> + }
> The upper bounds have been already checked in ethtool_set_ringparam() so
> that the two conditions can never be satisfied here.
>
> ...
>> +static int ionic_set_channels(struct net_device *netdev,
>> + struct ethtool_channels *ch)
>> +{
>> + struct lif *lif = netdev_priv(netdev);
>> + bool running;
>> +
>> + if (!ch->combined_count || ch->other_count ||
>> + ch->rx_count || ch->tx_count)
>> + return -EINVAL;
>> +
>> + if (ch->combined_count > lif->ionic->ntxqs_per_lif)
>> + return -EINVAL;
> This has been already checked in ethtool_set_channels().
That's what I get for copying from an existing driver. I'll check those
and clean them up.
>
> Michal Kubecek
Thanks!
sln
^ permalink raw reply
* Re: [PATCH net-next 02/18] ionic: Add hardware init and device commands
From: Shannon Nelson @ 2019-06-21 22:22 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev
In-Reply-To: <20190620215430.GK31306@lunn.ch>
On 6/20/19 2:54 PM, Andrew Lunn wrote:
> On Thu, Jun 20, 2019 at 01:24:08PM -0700, Shannon Nelson wrote:
>> + err = ionic_debugfs_add_dev(ionic);
>> + if (err) {
>> + dev_err(dev, "Cannot add device debugfs: %d , aborting\n", err);
>> + goto err_out_clear_drvdata;
>> + }
> Hi Shannon
>
> debugfs should not fail, since it is optional. debugfs failing should
> also not be considered fatal. And lastly, debugfs is not very liked by
> network people, so you should avoid it as much as possible. Use
> ethtool, lspci, devlink, etc.
Yes, we can drop this error check.
>
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +
>> +static int blob_open(struct inode *inode, struct file *filp)
>> +{
>> + filp->private_data = inode->i_private;
>> + return 0;
>> +}
>> +
>> +static ssize_t blob_read(struct file *filp, char __user *buffer,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct debugfs_blob_wrapper *blob = filp->private_data;
>> +
>> + if (*ppos >= blob->size)
>> + return 0;
>> + if (*ppos + count > blob->size)
>> + count = blob->size - *ppos;
>> +
>> + if (copy_to_user(buffer, blob->data + *ppos, count))
>> + return -EFAULT;
>> +
>> + *ppos += count;
>> +
>> + return count;
>> +}
>> +
>> +static ssize_t blob_write(struct file *filp, const char __user *buffer,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct debugfs_blob_wrapper *blob = filp->private_data;
>> +
>> + if (*ppos >= blob->size)
>> + return 0;
>> + if (*ppos + count > blob->size)
>> + count = blob->size - *ppos;
>> +
>> + if (copy_from_user(blob->data + *ppos, buffer, count))
>> + return -EFAULT;
>> +
>> + *ppos += count;
>> +
>> + return count;
>> +}
> Write is pretty much a no-no. We don't want proprietary user space
> tools manipulating the hardware/driver state.
Yep, this needs to come out.
>
>> +
>> +static const struct file_operations blob_fops = {
>> + .owner = THIS_MODULE,
>> + .open = blob_open,
>> + .read = blob_read,
>> + .write = blob_write,
>> +};
>> +
>> +struct dentry *debugfs_create_blob(const char *name, umode_t mode,
>> + struct dentry *parent,
>> + struct debugfs_blob_wrapper *blob)
>> +{
>> + return debugfs_create_file(name, mode | 0200, parent, blob,
>> + &blob_fops);
>> +}
>> +
>> +static const struct debugfs_reg32 dev_cmd_regs[] = {
>> + { .name = "db", .offset = 0, },
>> + { .name = "done", .offset = 4, },
>> + { .name = "cmd.word[0]", .offset = 8, },
>> + { .name = "cmd.word[1]", .offset = 12, },
>> + { .name = "cmd.word[2]", .offset = 16, },
>> + { .name = "cmd.word[3]", .offset = 20, },
>> + { .name = "cmd.word[4]", .offset = 24, },
>> + { .name = "cmd.word[5]", .offset = 28, },
>> + { .name = "cmd.word[6]", .offset = 32, },
>> + { .name = "cmd.word[7]", .offset = 36, },
>> + { .name = "cmd.word[8]", .offset = 40, },
>> + { .name = "cmd.word[9]", .offset = 44, },
>> + { .name = "cmd.word[10]", .offset = 48, },
>> + { .name = "cmd.word[11]", .offset = 52, },
>> + { .name = "cmd.word[12]", .offset = 56, },
>> + { .name = "cmd.word[13]", .offset = 60, },
>> + { .name = "cmd.word[14]", .offset = 64, },
>> + { .name = "cmd.word[15]", .offset = 68, },
>> + { .name = "comp.word[0]", .offset = 72, },
>> + { .name = "comp.word[1]", .offset = 76, },
>> + { .name = "comp.word[2]", .offset = 80, },
>> + { .name = "comp.word[3]", .offset = 84, },
>> +};
>> +
>> +int ionic_debugfs_add_dev_cmd(struct ionic *ionic)
>> +{
>> + struct debugfs_regset32 *dev_cmd_regset;
>> + struct device *dev = ionic->dev;
>> + struct dentry *dentry;
>> +
>> + dev_cmd_regset = devm_kzalloc(dev, sizeof(*dev_cmd_regset),
>> + GFP_KERNEL);
>> + if (!dev_cmd_regset)
>> + return -ENOMEM;
>> + dev_cmd_regset->regs = dev_cmd_regs;
>> + dev_cmd_regset->nregs = ARRAY_SIZE(dev_cmd_regs);
>> + dev_cmd_regset->base = ionic->idev.dev_cmd_regs;
>> +
>> + dentry = debugfs_create_regset32("dev_cmd", 0400,
>> + ionic->dentry, dev_cmd_regset);
>> + if (IS_ERR_OR_NULL(dentry))
>> + return PTR_ERR(dentry);
> ethtool -d seems like a more acceptable method for exporting
> registers.
Yes, that is one of the near future additions planned.
>
>> +static int identity_show(struct seq_file *seq, void *v)
>> +{
>> + struct ionic *ionic = seq->private;
>> + struct identity *ident = &ionic->ident;
>> + struct ionic_dev *idev = &ionic->idev;
>> +
>> + seq_printf(seq, "asic_type: 0x%x\n", idev->dev_info.asic_type);
>> + seq_printf(seq, "asic_rev: 0x%x\n", idev->dev_info.asic_rev);
>> + seq_printf(seq, "serial_num: %s\n", idev->dev_info.serial_num);
>> + seq_printf(seq, "fw_version: %s\n", idev->dev_info.fw_version);
>> + seq_printf(seq, "fw_status: 0x%x\n",
>> + ioread8(&idev->dev_info_regs->fw_status));
>> + seq_printf(seq, "fw_heartbeat: 0x%x\n",
>> + ioread32(&idev->dev_info_regs->fw_heartbeat));
> devlink just gained a much more flexible version of ethtool -i. Please
> remove all this and use that.
Yes, we intend to add a devlink interface, it just isn't in this first
patchset, which is already plenty big.
>
>> +int ionic_dev_setup(struct ionic *ionic)
>> +{
>> + struct ionic_dev_bar *bar = ionic->bars;
>> + unsigned int num_bars = ionic->num_bars;
>> + struct ionic_dev *idev = &ionic->idev;
>> + struct device *dev = ionic->dev;
>> + u32 sig;
>> +
>> + /* BAR0: dev_cmd and interrupts */
>> + if (num_bars < 1) {
>> + dev_info(dev, "No bars found, aborting\n");
> dev_err(), since this is fatal. The same for most of these dev_info()
> calls.
Will do
>
>> +enum os_type {
>> + IONIC_OS_TYPE_LINUX = 1,
>> + IONIC_OS_TYPE_WIN = 2,
>> + IONIC_OS_TYPE_DPDK = 3,
>> + IONIC_OS_TYPE_FREEBSD = 4,
>> + IONIC_OS_TYPE_IPXE = 5,
>> + IONIC_OS_TYPE_ESXI = 6,
>> +};
>> +
>> +/**
>> + * union drv_identity - driver identity information
>> + * @os_type: OS type (see enum os_type)
>> + * @os_dist: OS distribution, numeric format
>> + * @os_dist_str: OS distribution, string format
>> + * @kernel_ver: Kernel version, numeric format
>> + * @kernel_ver_str: Kernel version, string format
>> + * @driver_ver_str: Driver version, string format
>> + */
>> +union drv_identity {
>> + struct {
>> + __le32 os_type;
>> + __le32 os_dist;
>> + char os_dist_str[128];
>> + __le32 kernel_ver;
>> + char kernel_ver_str[32];
>> + char driver_ver_str[32];
>> + };
>> + __le32 words[512];
>> +};
>> +int ionic_identify(struct ionic *ionic)
>> +{
>> + struct identity *ident = &ionic->ident;
>> + struct ionic_dev *idev = &ionic->idev;
>> + size_t sz;
>> + int err;
>> +
>> + memset(ident, 0, sizeof(*ident));
>> +
>> + ident->drv.os_type = cpu_to_le32(IONIC_OS_TYPE_LINUX);
>> + ident->drv.os_dist = 0;
>> + strncpy(ident->drv.os_dist_str, utsname()->release,
>> + sizeof(ident->drv.os_dist_str) - 1);
>> + ident->drv.kernel_ver = cpu_to_le32(LINUX_VERSION_CODE);
>> + strncpy(ident->drv.kernel_ver_str, utsname()->version,
>> + sizeof(ident->drv.kernel_ver_str) - 1);
>> + strncpy(ident->drv.driver_ver_str, DRV_VERSION,
>> + sizeof(ident->drv.driver_ver_str) - 1);
> Creepy.
It does seem odd at first to be telling your NIC about the OS, but this
is becomes part of a larger whole and is used in our device support and
management.
Thanks,
sln
>
> Andrew
^ permalink raw reply
* Re: [PATCH net-next v7 08/11] ipv6: Dump route exceptions if requested
From: David Ahern @ 2019-06-21 22:21 UTC (permalink / raw)
To: Stefano Brivio, David Miller
Cc: Jianlin Shi, Wei Wang, Martin KaFai Lau, Eric Dumazet,
Matti Vaittinen, netdev
In-Reply-To: <4af136ad187cdd167bac5effcf7e7b2932db87b9.1561131177.git.sbrivio@redhat.com>
On 6/21/19 9:45 AM, Stefano Brivio wrote:
> Since commit 2b760fcf5cfb ("ipv6: hook up exception table to store dst
> cache"), route exceptions reside in a separate hash table, and won't be
> found by walking the FIB, so they won't be dumped to userspace on a
> RTM_GETROUTE message.
>
> This causes 'ip -6 route list cache' and 'ip -6 route flush cache' to
> have no function anymore:
>
> # ip -6 route get fc00:3::1
> fc00:3::1 via fc00:1::2 dev veth_A-R1 src fc00:1::1 metric 1024 expires 539sec mtu 1400 pref medium
> # ip -6 route get fc00:4::1
> fc00:4::1 via fc00:2::2 dev veth_A-R2 src fc00:2::1 metric 1024 expires 536sec mtu 1500 pref medium
> # ip -6 route list cache
> # ip -6 route flush cache
> # ip -6 route get fc00:3::1
> fc00:3::1 via fc00:1::2 dev veth_A-R1 src fc00:1::1 metric 1024 expires 520sec mtu 1400 pref medium
> # ip -6 route get fc00:4::1
> fc00:4::1 via fc00:2::2 dev veth_A-R2 src fc00:2::1 metric 1024 expires 519sec mtu 1500 pref medium
>
> because iproute2 lists cached routes using RTM_GETROUTE, and flushes them
> by listing all the routes, and deleting them with RTM_DELROUTE one by one.
>
> If cached routes are requested using the RTM_F_CLONED flag together with
> strict checking, or if no strict checking is requested (and hence we can't
> consistently apply filters), look up exceptions in the hash table
> associated with the current fib6_info in rt6_dump_route(), and, if present
> and not expired, add them to the dump.
>
> We might be unable to dump all the entries for a given node in a single
> message, so keep track of how many entries were handled for the current
> node in fib6_walker, and skip that amount in case we start from the same
> partially dumped node.
>
> When a partial dump restarts, as the starting node might change when
> 'sernum' changes, we have no guarantee that we need to skip the same
> amount of in-node entries. Therefore, we need two counters, and we need to
> zero the in-node counter if the node from which the dump is resumed
> differs.
>
> Note that, with the current version of iproute2, this only fixes the
> 'ip -6 route list cache': on a flush command, iproute2 doesn't pass
> RTM_F_CLONED and, due to this inconsistency, 'ip -6 route flush cache' is
> still unable to fetch the routes to be flushed. This will be addressed in
> a patch for iproute2.
>
> To flush cached routes, a procfs entry could be introduced instead: that's
> how it works for IPv4. We already have a rt6_flush_exception() function
> ready to be wired to it. However, this would not solve the issue for
> listing.
>
...
>
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Fixes: 2b760fcf5cfb ("ipv6: hook up exception table to store dst cache")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> include/net/ip6_fib.h | 1 +
> include/net/ip6_route.h | 2 +-
> net/ipv6/ip6_fib.c | 12 ++++-
> net/ipv6/route.c | 114 ++++++++++++++++++++++++++++++++++++----
> 4 files changed, 116 insertions(+), 13 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next v7 04/11] ipv4: Dump route exceptions if requested
From: David Ahern @ 2019-06-21 22:19 UTC (permalink / raw)
To: Stefano Brivio, David Miller
Cc: Jianlin Shi, Wei Wang, Martin KaFai Lau, Eric Dumazet,
Matti Vaittinen, netdev
In-Reply-To: <8d3b68cd37fb5fddc470904cdd6793fcf480c6c1.1561131177.git.sbrivio@redhat.com>
On 6/21/19 9:45 AM, Stefano Brivio wrote:
> Since commit 4895c771c7f0 ("ipv4: Add FIB nexthop exceptions."), cached
> exception routes are stored as a separate entity, so they are not dumped
> on a FIB dump, even if the RTM_F_CLONED flag is passed.
>
> This implies that the command 'ip route list cache' doesn't return any
> result anymore.
>
> If the RTM_F_CLONED is passed, and strict checking requested, retrieve
> nexthop exception routes and dump them. If no strict checking is
> requested, filtering can't be performed consistently: dump everything in
> that case.
>
> With this, we need to add an argument to the netlink callback in order to
> track how many entries were already dumped for the last leaf included in
> a partial netlink dump.
>
> A single additional argument is sufficient, even if we traverse logically
> nested structures (nexthop objects, hash table buckets, bucket chains): it
> doesn't matter if we stop in the middle of any of those, because they are
> always traversed the same way. As an example, s_i values in [], s_fa
> values in ():
>
> node (fa) #1 [1]
> nexthop #1
> bucket #1 -> #0 in chain (1)
> bucket #2 -> #0 in chain (2) -> #1 in chain (3) -> #2 in chain (4)
> bucket #3 -> #0 in chain (5) -> #1 in chain (6)
>
> nexthop #2
> bucket #1 -> #0 in chain (7) -> #1 in chain (8)
> bucket #2 -> #0 in chain (9)
> --
> node (fa) #2 [2]
> nexthop #1
> bucket #1 -> #0 in chain (1) -> #1 in chain (2)
> bucket #2 -> #0 in chain (3)
>
> it doesn't matter if we stop at (3), (4), (7) for "node #1", or at (2)
> for "node #2": walking flattens all that.
>
> It would even be possible to drop the distinction between the in-tree
> (s_i) and in-node (s_fa) counter, but a further improvement might
> advise against this. This is only as accurate as the existing tracking
> mechanism for leaves: if a partial dump is restarted after exceptions
> are removed or expired, we might skip some non-dumped entries.
...
>
> Fixes: 4895c771c7f0 ("ipv4: Add FIB nexthop exceptions.")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> include/net/route.h | 4 +++
> net/ipv4/fib_trie.c | 44 +++++++++++++++++++--------
> net/ipv4/route.c | 73 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 108 insertions(+), 13 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next 01/18] ionic: Add basic framework for IONIC Network device driver
From: Shannon Nelson @ 2019-06-21 22:13 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev
In-Reply-To: <20190620212447.GJ31306@lunn.ch>
On 6/20/19 2:24 PM, Andrew Lunn wrote:
Hi Andrew, thanks for your time and comments. Responses below...
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
>> +
>> +#ifndef _IONIC_H_
>> +#define _IONIC_H_
>> +
>> +#define DRV_NAME "ionic"
>> +#define DRV_DESCRIPTION "Pensando Ethernet NIC Driver"
>> +#define DRV_VERSION "0.11.0-k"
> DRV_VERSION is pretty useless. What you really want to know is the
> kernel git tree and commit. The big distributions might backport this
> version of the driver back to the old kernel with a million
> patches. At which point 0.11.0-k tells you nothing much.
Yes, any version numbering thing from the big distros is put into
question, but I find this number useful to me for tracking what has been
put into the upstream kernel. This plus the full kernel version gives
me a pretty good idea of what I'm looking at.
>> +
>> +// TODO: register these with the official include/linux/pci_ids.h
>> +#define PCI_VENDOR_ID_PENSANDO 0x1dd8
> That file has a comment:
>
> * Do not add new entries to this file unless the definitions
> * are shared between multiple drivers.
>
> Is it going to be shared?
Yes, there is an instance of sharing planned.
>
> +
>> +#define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF 0x1002
>> +#define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF 0x1003
>> +#define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT 0x1004
>> +
>> +#define IONIC_SUBDEV_ID_NAPLES_25 0x4000
>> +#define IONIC_SUBDEV_ID_NAPLES_100_4 0x4001
>> +#define IONIC_SUBDEV_ID_NAPLES_100_8 0x4002
>> +
>> +struct ionic {
>> + struct pci_dev *pdev;
>> + struct device *dev;
>> +};
>> +
>> +#endif /* _IONIC_H_ */
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus.h b/drivers/net/ethernet/pensando/ionic/ionic_bus.h
>> new file mode 100644
>> index 000000000000..94ba0afc6f38
>> --- /dev/null
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus.h
>> @@ -0,0 +1,10 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
>> +
>> +#ifndef _IONIC_BUS_H_
>> +#define _IONIC_BUS_H_
>> +
>> +int ionic_bus_register_driver(void);
>> +void ionic_bus_unregister_driver(void);
>> +
>> +#endif /* _IONIC_BUS_H_ */
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> new file mode 100644
>> index 000000000000..ab6206c162d4
>> --- /dev/null
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> @@ -0,0 +1,61 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
>> +
>> +#include <linux/module.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/etherdevice.h>
>> +#include <linux/pci.h>
>> +
>> +#include "ionic.h"
>> +#include "ionic_bus.h"
>> +
>> +/* Supported devices */
>> +static const struct pci_device_id ionic_id_table[] = {
>> + { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF) },
>> + { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF) },
>> + { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT) },
>> + { 0, } /* end of table */
>> +};
>> +MODULE_DEVICE_TABLE(pci, ionic_id_table);
>> +
>> +static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct ionic *ionic;
>> +
>> + ionic = devm_kzalloc(dev, sizeof(*ionic), GFP_KERNEL);
>> + if (!ionic)
>> + return -ENOMEM;
>> +
>> + ionic->pdev = pdev;
>> + pci_set_drvdata(pdev, ionic);
>> + ionic->dev = dev;
>> + dev_info(ionic->dev, "attached\n");
> probed would be more accurate. But in general, please avoid all but
> the minimum of such info messages.
Sure
>
>> +
>> + return 0;
>> +}
>> +
>> +static void ionic_remove(struct pci_dev *pdev)
>> +{
>> + struct ionic *ionic = pci_get_drvdata(pdev);
>> +
>> + pci_set_drvdata(pdev, NULL);
>> + dev_info(ionic->dev, "removed\n");
> Not very useful dev_info().
It has been useful in testing, but it can go away.
>
> Also, i think the core will NULL out the drive data for you. But you
> should check.
I'll check.
>> +}
>> +
>> +static struct pci_driver ionic_driver = {
>> + .name = DRV_NAME,
>> + .id_table = ionic_id_table,
>> + .probe = ionic_probe,
>> + .remove = ionic_remove,
>> +};
>> +
>> +int ionic_bus_register_driver(void)
>> +{
>> + return pci_register_driver(&ionic_driver);
>> +}
>> +
>> +void ionic_bus_unregister_driver(void)
>> +{
>> + pci_unregister_driver(&ionic_driver);
>> +}
> It looks like you can use module_pci_driver() and remove a lot of
> boilerplate.
Thanks, I'll look at that.
Cheers,
sln
>
> Andrew
^ permalink raw reply
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Russell King - ARM Linux admin @ 2019-06-21 22:09 UTC (permalink / raw)
To: Vivien Didelot
Cc: Florian Fainelli, netdev, idosch, Jiri Pirko, andrew, davem
In-Reply-To: <20190621172952.GB9284@t480s.localdomain>
On Fri, Jun 21, 2019 at 05:29:52PM -0400, Vivien Didelot wrote:
> Russell, Ido, Florian, so far I understand that a multicast-unaware
> bridge must flood unknown traffic everywhere (CPU included);
^
multicast
> and a multicast-aware bridge must only flood its ports if their
^
unknown multicast traffic to
> mcast_flood is on, and known traffic targeting the bridge must be
> offloaded accordingly. Do you guys agree with this?
I don't see a problem with that with the clarification that we're
talking about multicast packets here.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Vivien Didelot @ 2019-06-21 21:29 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, idosch, Jiri Pirko, linux, andrew, davem
In-Reply-To: <5d653a4d-3270-8e53-a5e0-88ea5e7a4d3f@gmail.com>
On Thu, 20 Jun 2019 19:24:47 -0700, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > This patch adds support for enabling or disabling the flooding of
> > unknown multicast traffic on the CPU ports, depending on the value
> > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute.
> >
> > This allows the user to prevent the CPU to be flooded with a lot of
> > undesirable traffic that the network stack needs to filter in software.
> >
> > The bridge has multicast snooping enabled by default, hence CPU ports
> > aren't bottlenecked with arbitrary network applications anymore.
> > But this can be an issue in some scenarios such as pinging the bridge's
> > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to
> > 0 would restore unknown multicast flooding and thus fix ICMPv6. As
> > an alternative, enabling multicast_querier would program the bridge
> > address into the switch.
> From what I can read from mlxsw, we should probably also implement the
> SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent.
>
> Since the attribute MC_DISABLED is on the bridge master, we should also
> iterate over the list of switch ports being a member of that bridge and
> change their flooding attribute, taking into account whether
> BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing
> what mlxsw does here again...
Ouch, doesn't sound like what a driver should be doing :-(
Ido, I cannot find documentation for multicast_snooping or MC_DISABLED
and the naming isn't clear. Can this be considered as an equivalent
of mcast_flood but targeting the bridge device itself, describing
whether the bridge is interested or not in unknown multicast traffic?
> Once you act on the user-facing ports, you might be able to leave the
> CPU port flooding unconditionally, since it would only "flood" the CPU
> port either because an user-facing port has BR_MCAST_FLOOD set, or
> because this is known MC traffic that got programmed via the bridge's
> MDB. Would that work?
You may want the machine or network connected behind a bridge port
to be flooded with unknown multicast traffic, without having the
CPU conduit clogged up with this traffic. So these are two distinct
settings for me.
The only scenario I can think of needing the CPU to be flooded is if
there's a non-DSA port in the bridge maybe. But IMHO this should be
handled by the bridge, offloading or not the appropriate attribute.
> On a higher level, I really wish we did not have to re-implement a lot
> of identical or similar logic in each switch drivers and had a more
> central model of what is behaviorally expected.
I couldn't agree more, ethernet switch drivers should only offload
the notified bridge configuration, not noodling their own logic...
Russell, Ido, Florian, so far I understand that a multicast-unaware
bridge must flood unknown traffic everywhere (CPU included);
and a multicast-aware bridge must only flood its ports if their
mcast_flood is on, and known traffic targeting the bridge must be
offloaded accordingly. Do you guys agree with this?
Thanks,
Vivien
^ permalink raw reply
* [PATCH] bnx2x: Prevent ptp_task to be rescheduled indefinitely
From: Guilherme G. Piccoli @ 2019-06-21 21:26 UTC (permalink / raw)
To: GR-everest-linux-l2, netdev; +Cc: aelior, skalluru, gpiccoli, jay.vosburgh
Currently bnx2x ptp worker tries to read a register with timestamp
information in case of TX packet timestamping and in case it fails,
the routine reschedules itself indefinitely. This was reported as a
kworker always at 100% of CPU usage, which was narrowed down to be
bnx2x ptp_task.
By following the ioctl handler, we could narrow down the problem to an
NTP tool (chrony) requesting HW timestamping from bnx2x NIC with RX
filter zeroed; this isn't reproducible for example with linuxptp since
this tool request a supported RX filter. It seems the NIC HW timestamp
mechanism cannot work well with RX_FILTER_NONE - in driver's PTP filter
initialization routine, when there's not a supported filter request the
function does not perform a specific register write to the adapter.
This patch addresses the problem of the everlasting reschedule of the
ptp worker by limiting that to 3 attempts (the first one plus two
reschedules), in order to prevent the unbound resource consumption
from the driver. It's not correct behavior for a driver to not take
into account potential problems in a routine reading a device register,
be it an invalid RX filter (leading to a non-functional HW clock) or
even a potential device FW issue causing the register value to be wrong,
hence we believe the fix is relevant to ensure proper driver behavior.
This has no functional change in the succeeding path of the HW
timestamping code in the driver, only portion of code it changes
is the error path for TX timestamping. It was tested using both
linuxptp and chrony.
Reported-and-tested-by: Przemyslaw Hausman <przemyslaw.hausman@canonical.com>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 1 +
.../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 1 +
.../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 18 +++++++++++++-----
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 6026b53137aa..349965135227 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -1838,6 +1838,7 @@ struct bnx2x {
bool timecounter_init_done;
struct sk_buff *ptp_tx_skb;
unsigned long ptp_tx_start;
+ u8 ptp_retry_count;
bool hwtstamp_ioctl_called;
u16 tx_type;
u16 rx_filter;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 008ad0ca89ba..990ec049f357 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -3865,6 +3865,7 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
/* schedule check for Tx timestamp */
bp->ptp_tx_skb = skb_get(skb);
bp->ptp_tx_start = jiffies;
+ bp->ptp_retry_count = 0;
schedule_work(&bp->ptp_task);
}
}
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 03ac10b1cd1e..872ae672faaa 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -15233,16 +15233,24 @@ static void bnx2x_ptp_task(struct work_struct *work)
memset(&shhwtstamps, 0, sizeof(shhwtstamps));
shhwtstamps.hwtstamp = ns_to_ktime(ns);
skb_tstamp_tx(bp->ptp_tx_skb, &shhwtstamps);
- dev_kfree_skb_any(bp->ptp_tx_skb);
- bp->ptp_tx_skb = NULL;
-
DP(BNX2X_MSG_PTP, "Tx timestamp, timestamp cycles = %llu, ns = %llu\n",
timestamp, ns);
+ goto clear;
} else {
DP(BNX2X_MSG_PTP, "There is no valid Tx timestamp yet\n");
- /* Reschedule to keep checking for a valid timestamp value */
- schedule_work(&bp->ptp_task);
+ /* Reschedule twice to check again for a valid timestamp */
+ if (++bp->ptp_retry_count < 3) {
+ schedule_work(&bp->ptp_task);
+ return;
+ }
+ DP(BNX2X_MSG_PTP, "Gave up Tx timestamp, register read %u\n", val_seq);
+ netdev_warn_once(bp->dev,
+ "Gave up Tx timestamp, register read %u\n", val_seq);
}
+clear:
+ dev_kfree_skb_any(bp->ptp_tx_skb);
+ bp->ptp_tx_skb = NULL;
+ bp->ptp_retry_count = 0;
}
void bnx2x_set_rx_ts(struct bnx2x *bp, struct sk_buff *skb)
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2 bpf-next 0/7] libbpf: add tracing attach APIs
From: Stanislav Fomichev @ 2019-06-21 21:21 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: andrii.nakryiko, ast, daniel, bpf, netdev, kernel-team
In-Reply-To: <20190621045555.4152743-1-andriin@fb.com>
On 06/20, Andrii Nakryiko wrote:
> This patchset adds the following APIs to allow attaching BPF programs to
> tracing entities:
> - bpf_program__attach_perf_event for attaching to any opened perf event FD,
> allowing users full control;
> - bpf_program__attach_kprobe for attaching to kernel probes (both entry and
> return probes);
> - bpf_program__attach_uprobe for attaching to user probes (both entry/return);
> - bpf_program__attach_tracepoint for attaching to kernel tracepoints;
> - bpf_program__attach_raw_tracepoint for attaching to raw kernel tracepoint
> (wrapper around bpf_raw_tracepoint_open);
>
> This set of APIs makes libbpf more useful for tracing applications.
>
> Pre-patch #1 makes internal libbpf_strerror_r helper function work w/ negative
> error codes, lifting the burder off callers to keep track of error sign.
> Patch #2 adds attach_perf_event, which is the base for all other APIs.
> Patch #3 adds kprobe/uprobe APIs.
> Patch #4 adds tracepoint/raw_tracepoint APIs.
> Patch #5 converts one existing test to use attach_perf_event.
> Patch #6 adds new kprobe/uprobe tests.
> Patch #7 converts all the selftests currently using tracepoint to new APIs.
>
> v1->v2:
> - preserve errno before close() call (Stanislav);
> - use libbpf_perf_event_disable_and_close in selftest (Stanislav);
> - remove unnecessary memset (Stanislav);
Reviewed-by: Stanislav Fomichev <sdf@google.com>
Thanks!
> Andrii Nakryiko (7):
> libbpf: make libbpf_strerror_r agnostic to sign of error
> libbpf: add ability to attach/detach BPF to perf event
> libbpf: add kprobe/uprobe attach API
> libbpf: add tracepoint/raw tracepoint attach API
> selftests/bpf: switch test to new attach_perf_event API
> selftests/bpf: add kprobe/uprobe selftests
> selftests/bpf: convert existing tracepoint tests to new APIs
>
> tools/lib/bpf/libbpf.c | 346 ++++++++++++++++++
> tools/lib/bpf/libbpf.h | 17 +
> tools/lib/bpf/libbpf.map | 6 +
> tools/lib/bpf/str_error.c | 2 +-
> .../selftests/bpf/prog_tests/attach_probe.c | 151 ++++++++
> .../bpf/prog_tests/stacktrace_build_id.c | 49 +--
> .../bpf/prog_tests/stacktrace_build_id_nmi.c | 24 +-
> .../selftests/bpf/prog_tests/stacktrace_map.c | 42 +--
> .../bpf/prog_tests/stacktrace_map_raw_tp.c | 14 +-
> .../bpf/prog_tests/task_fd_query_rawtp.c | 10 +-
> .../bpf/prog_tests/task_fd_query_tp.c | 51 +--
> .../bpf/prog_tests/tp_attach_query.c | 56 +--
> .../selftests/bpf/progs/test_attach_probe.c | 55 +++
> 13 files changed, 651 insertions(+), 172 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_probe.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_attach_probe.c
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH net-next] ipv6: Convert gateway validation to use fib6_info
From: David Ahern @ 2019-06-21 21:04 UTC (permalink / raw)
To: Wei Wang, David Ahern
Cc: David S . Miller, Linux Kernel Network Developers,
Martin KaFai Lau
In-Reply-To: <CAEA6p_BUSFUCJJ_WsAAM2JRhQBBHjUepNZPpFX6DrTSCancD_g@mail.gmail.com>
On 6/20/19 5:43 PM, Wei Wang wrote:
> I am not very convinced that fib6_lookup() could be equivalent to
> rt6_lookup(). Specifically, rt6_lookup() calls rt6_device_match()
> while fib6_lookup() calls rt6_select() to match the oif. From a brief
> glance, it does seem to be similar, especially considering that saddr
> is NULL. So it probably is OK?
When you remove the rt6_check_neigh call since RT6_LOOKUP_F_REACHABLE is
not set that removes the RT6_NUD_FAIL_DO_RR return and round-robin
logic. I am reasonably confident that given the use case - validate the
gateway and optionally given the device - it is the same.
rt6_select is much more complicated than rt6_device_match, so there is a
small possibility that in some corner case gateway validation fails /
succeeds with fib6_table_lookup where it would succeed / fail with
ip6_pol_route_lookup. But, ip6_pol_route and fib6_table_lookup is the
code path actually used for packet Rx and Tx, so it seems to me to be
the more proper one for gateway validation.
I will send a v2 with idev change you mentioned.
^ permalink raw reply
* Re: Removing skb_orphan() from ip_rcv_core()
From: Florian Westphal @ 2019-06-21 20:59 UTC (permalink / raw)
To: Joe Stringer
Cc: Eric Dumazet, Florian Westphal, netdev, john fastabend,
Daniel Borkmann, Lorenz Bauer, Jakub Sitnicki, Paolo Abeni
In-Reply-To: <CAOftzPisP-3jN8drC6RXcTigXJjdwEnvTRvTHR-Kv4LKn4rhQQ@mail.gmail.com>
Joe Stringer <joe@wand.net.nz> wrote:
> As discussed during LSFMM, I've been looking at adding something like
> an `skb_sk_assign()` helper to BPF so that logic similar to TPROXY can
> be implemented with integration into other BPF logic, however
> currently any attempts to do so are blocked by the skb_orphan() call
> in ip_rcv_core() (which will effectively ignore any socket assign
> decision made by the TC BPF program).
>
> Recently I was attempting to remove the skb_orphan() call, and I've
> been trying different things but there seems to be some context I'm
> missing. Here's the core of the patch:
>
> diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> index ed97724c5e33..16aea980318a 100644
> --- a/net/ipv4/ip_input.c
> +++ b/net/ipv4/ip_input.c
> @@ -500,8 +500,6 @@ static struct sk_buff *ip_rcv_core(struct sk_buff
> *skb, struct net *net)
> memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
> IPCB(skb)->iif = skb->skb_iif;
>
> - /* Must drop socket now because of tproxy. */
> - skb_orphan(skb);
>
> return skb;
>
> The statement that the socket must be dropped because of tproxy
> doesn't make sense to me, because the PRE_ROUTING hook is hit after
> this, which will call into the tproxy logic and eventually
> nf_tproxy_assign_sock() which already does the skb_orphan() itself.
in comment: s/tproxy/skb_steal_sock/
at least thats what I concluded a few years ago when I looked into
the skb_oprhan() need.
IIRC some device drivers use skb->sk for backpressure, so without this
non-tcp socket would be stolen by skb_steal_sock.
We also recently removed skb orphan when crossing netns:
commit 9c4c325252c54b34d53b3d0ffd535182b744e03d
Author: Flavio Leitner <fbl@redhat.com>
skbuff: preserve sock reference when scrubbing the skb.
So thats another case where this orphan is needed.
What could be done is adding some way to delay/defer the orphaning
further, but we would need at the very least some annotation for
skb_steal_sock to know when the skb->sk is really from TPROXY or
if it has to orphan.
Same for the safety check in the forwarding path.
Netfilter modules need o be audited as well, they might make assumptions
wrt. skb->sk being inet sockets (set by local stack or early demux).
> However, if I drop these lines then I end up causing sockets to
> release references too many times. Seems like if we don't orphan the
> skb here, then later logic assumes that we have one more reference
> than we actually have, and decrements the count when it shouldn't
> (perhaps the skb_steal_sock() call in __inet_lookup_skb() which seems
> to assume we always have a reference to the socket?)
We might be calling the wrong destructor (i.e., the one set by tcp
receive instead of the one set at tx time)?
^ permalink raw reply
* [PATCH] xsk: sample kernel code is now in libbpf
From: Eric Leblond @ 2019-06-21 20:13 UTC (permalink / raw)
To: netdev, bpf; +Cc: Eric Leblond
Fix documentation that mention xdpsock_kern.c which has been
replaced by code embedded in libbpf.
Signed-off-by: Eric Leblond <eric@regit.org>
---
Documentation/networking/af_xdp.rst | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/Documentation/networking/af_xdp.rst b/Documentation/networking/af_xdp.rst
index e14d7d40fc75..83dddc20f5d6 100644
--- a/Documentation/networking/af_xdp.rst
+++ b/Documentation/networking/af_xdp.rst
@@ -220,7 +220,21 @@ Usage
In order to use AF_XDP sockets there are two parts needed. The
user-space application and the XDP program. For a complete setup and
usage example, please refer to the sample application. The user-space
-side is xdpsock_user.c and the XDP side xdpsock_kern.c.
+side is xdpsock_user.c and the XDP side is part of libbpf.
+
+The XDP code sample included in tools/lib/bpf/xsk.c is the following::
+
+ SEC("xdp_sock") int xdp_sock_prog(struct xdp_md *ctx)
+ {
+ int index = ctx->rx_queue_index;
+
+ // A set entry here means that the correspnding queue_id
+ // has an active AF_XDP socket bound to it.
+ if (bpf_map_lookup_elem(&xsks_map, &index))
+ return bpf_redirect_map(&xsks_map, index, 0);
+
+ return XDP_PASS;
+ }
Naive ring dequeue and enqueue could look like this::
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next] doc: phy: document some PHY_INTERFACE_MODE_xxx settings
From: Andrew Lunn @ 2019-06-21 20:11 UTC (permalink / raw)
To: Russell King
Cc: Florian Fainelli, Heiner Kallweit, David S. Miller,
Jonathan Corbet, netdev, linux-doc
In-Reply-To: <E1heL0P-00075z-An@rmk-PC.armlinux.org.uk>
On Fri, Jun 21, 2019 at 03:59:09PM +0100, Russell King wrote:
> There seems to be some confusion surrounding three PHY interface modes,
> specifically 1000BASE-X, 2500BASE-X and SGMII. Add some documentation
> to phylib detailing precisely what these interface modes refer to.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Willem de Bruijn @ 2019-06-21 20:06 UTC (permalink / raw)
To: Neil Horman; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <20190621191823.GD21895@hmswarspite.think-freely.org>
On Fri, Jun 21, 2019 at 3:18 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Fri, Jun 21, 2019 at 02:31:17PM -0400, Willem de Bruijn wrote:
> > On Fri, Jun 21, 2019 at 12:42 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Thu, Jun 20, 2019 at 11:16:13AM -0400, Willem de Bruijn wrote:
> > > > On Thu, Jun 20, 2019 at 10:24 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > >
> > > > > On Thu, Jun 20, 2019 at 09:41:30AM -0400, Willem de Bruijn wrote:
> > > > > > On Wed, Jun 19, 2019 at 4:26 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > > >
> > > > > > > When an application is run that:
> > > > > > > a) Sets its scheduler to be SCHED_FIFO
> > > > > > > and
> > > > > > > b) Opens a memory mapped AF_PACKET socket, and sends frames with the
> > > > > > > MSG_DONTWAIT flag cleared, its possible for the application to hang
> > > > > > > forever in the kernel. This occurs because when waiting, the code in
> > > > > > > tpacket_snd calls schedule, which under normal circumstances allows
> > > > > > > other tasks to run, including ksoftirqd, which in some cases is
> > > > > > > responsible for freeing the transmitted skb (which in AF_PACKET calls a
> > > > > > > destructor that flips the status bit of the transmitted frame back to
> > > > > > > available, allowing the transmitting task to complete).
> > > > > > >
> > > > > > > However, when the calling application is SCHED_FIFO, its priority is
> > > > > > > such that the schedule call immediately places the task back on the cpu,
> > > > > > > preventing ksoftirqd from freeing the skb, which in turn prevents the
> > > > > > > transmitting task from detecting that the transmission is complete.
> > > > > > >
> > > > > > > We can fix this by converting the schedule call to a completion
> > > > > > > mechanism. By using a completion queue, we force the calling task, when
> > > > > > > it detects there are no more frames to send, to schedule itself off the
> > > > > > > cpu until such time as the last transmitted skb is freed, allowing
> > > > > > > forward progress to be made.
> > > > > > >
> > > > > > > Tested by myself and the reporter, with good results
> > > > > > >
> > > > > > > Appies to the net tree
> > > > > > >
> > > > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > > > > Reported-by: Matteo Croce <mcroce@redhat.com>
> > > > > > > CC: "David S. Miller" <davem@davemloft.net>
> > > > > > > ---
> > > > > >
> > > > > > This is a complex change for a narrow configuration. Isn't a
> > > > > > SCHED_FIFO process preempting ksoftirqd a potential problem for other
> > > > > > networking workloads as well? And the right configuration to always
> > > > > > increase ksoftirqd priority when increasing another process's
> > > > > > priority? Also, even when ksoftirqd kicks in, isn't some progress
> > > > > > still made on the local_bh_enable reached from schedule()?
> > > > > >
> > > > >
> > > > > A few questions here to answer:
> > > >
> > > > Thanks for the detailed explanation.
> > > >
> > > > > Regarding other protocols having this problem, thats not the case, because non
> > > > > packet sockets honor the SK_SNDTIMEO option here (i.e. they sleep for a period
> > > > > of time specified by the SNDTIMEO option if MSG_DONTWAIT isn't set. We could
> > > > > certainly do that, but the current implementation doesn't (opting instead to
> > > > > wait indefinately until the respective packet(s) have transmitted or errored
> > > > > out), and I wanted to maintain that behavior. If there is consensus that packet
> > > > > sockets should honor SNDTIMEO, then I can certainly do that.
> > > > >
> > > > > As for progress made by calling local_bh_enable, My read of the code doesn't
> > > > > have the scheduler calling local_bh_enable at all. Instead schedule uses
> > > > > preempt_disable/preempt_enable_no_resched() to gain exlcusive access to the cpu,
> > > > > which ignores pending softirqs on re-enablement.
> > > >
> > > > Ah, I'm mistaken there, then.
> > > >
> > > > > Perhaps that needs to change,
> > > > > but I'm averse to making scheduler changes for this (the aforementioned concern
> > > > > about complex changes for a narrow use case)
> > > > >
> > > > > Regarding raising the priority of ksoftirqd, that could be a solution, but the
> > > > > priority would need to be raised to a high priority SCHED_FIFO parameter, and
> > > > > that gets back to making complex changes for a narrow problem domain
> > > > >
> > > > > As for the comlexity of the of the solution, I think this is, given your
> > > > > comments the least complex and intrusive change to solve the given problem.
> > > >
> > > > Could it be simpler to ensure do_softirq() gets run here? That would
> > > > allow progress for this case.
> > > >
> > > > > We
> > > > > need to find a way to force the calling task off the cpu while the asynchronous
> > > > > operations in the transmit path complete, and we can do that this way, or by
> > > > > honoring SK_SNDTIMEO. I'm fine with doing the latter, but I didn't want to
> > > > > alter the current protocol behavior without consensus on that.
> > > >
> > > > In general SCHED_FIFO is dangerous with regard to stalling other
> > > > progress, incl. ksoftirqd. But it does appear that this packet socket
> > > > case is special inside networking in calling schedule() directly here.
> > > >
> > > > If converting that, should it convert to logic more akin to other
> > > > sockets, like sock_wait_for_wmem? I haven't had a chance to read up on
> > > > the pros and cons of completion here yet, sorry. Didn't want to delay
> > > > responding until after I get a chance.
> > > >
> > > So, I started looking at implementing SOCK_SNDTIMEO for this patch, and
> > > something occured to me....We still need a mechanism to block in tpacket_snd.
> > > That is to say, other protocol use SK_SNDTIMEO to wait for socket memory to
> > > become available, and that requirement doesn't exist for memory mapped sockets
> > > in AF_PACKET (which tpacket_snd implements the kernel side for). We have memory
> > > mapped frame buffers, which we marshall with an otherwise empty skb, and just
> > > send that (i.e. no waiting on socket memory, we just product an error if we
> > > don't have enough ram to allocate an sk_buff). Given that, we only ever need to
> > > wait for a frame to complete transmission, or get freed in an error path further
> > > down the stack. This probably explains why SK_SNDTIMEO doesn't exist for
> > > AF_PACKET.
> >
> > SNDTIMEO behavior would still be useful: to wait for frame slot to
> > become available, but only up to a timeout?
> >
> Ok, thats fair. To be clear, memory_mapped packets aren't waiting here for a
> frame to become available for sending (thats done in userspace, where the
> application checks a specific memory location for the TP_AVAILABLE status to be
> set, so a new frame can be written). tpacket_snd is wating for the transmission
> of a specific frame to complete the transmit action, which is a different thing.
Right. Until this report I was actually not even aware of this
behavior without MSG_DONTWAIT.
Though the wait is not for a specific frame, right? Wait as long as
the pending_refcnt, which is incremented on every loop.
> Still, I suppose theres no reason we couldn't contrain that wait on a timeout
> set by SK_SNDTIMEO
>
> > To be clear, adding that is not a prerequisite for fixing this
> > specific issue, of course. It would just be nice if the one happens to
> > be fixed by adding the other.
> >
> > My main question is wrt the implementation details of struct
> > completion. Without dynamic memory allocation,
> > sock_wait_for_wmem/sk_stream_write_space obviously does not make
> > sense. But should we still use sk_wq and more importantly does this
> > need the same wait semantics (TASK_INTERRUPTIBLE) and does struct
> > completion give those?
> >
> I suppose we could overload its use here, but I would be worried that such an
> overload would be confusing. Nominally, sk_wq is used, as you note, to block
> sockets whose allocated send space is full, until such time as enough frames
> have been sent to make space for the next write. In this scenario, we already
> know we have space to send a frame (by virtue of the fact that we are executing
> in tpakcet_snd, which is only called after userspace has written a frame to the
> memory mapped buffer already allocated for frame transmission). In this case we
> are simply waiting for the last frame that we have sent to complete
> transmission, at which point we can look to see if more frames are available to
> send, or return from the system call. I'm happy to take an alternate consensus
> into account, but for the sake of clarity I think I would rather use the
> completion queue, as it makes clear the correlation between the waiter and the
> event we are waiting on.
That will be less likely to have unexpected side-effects. Agreed.
Thanks for the explanation.
Only last question, does it have the right wait behavior? Should this
be wait_for_completion_interruptible?
^ permalink raw reply
* Re: [PATCH v2 net-next 5/5] ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF
From: kbuild test robot @ 2019-06-21 20:05 UTC (permalink / raw)
To: Wei Wang
Cc: kbuild-all, David Miller, netdev, Eric Dumazet, Mahesh Bandewar,
Martin KaFai Lau, David Ahern, Wei Wang
In-Reply-To: <20190619223158.35829-6-tracywwnj@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]
Hi Wei,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Wei-Wang/ipv6-avoid-taking-refcnt-on-dst-during-route-lookup/20190621-195237
config: x86_64-randconfig-u0-06220153 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/net//ethernet/broadcom/cnic.c:41:0:
include/net/ip6_route.h: In function 'ip6_route_output_flags':
>> include/net/ip6_route.h:103:13: error: 'struct net' has no member named 'ipv6'
dst = &net->ipv6.ip6_null_entry->dst;
^
vim +103 include/net/ip6_route.h
80
81 void ip6_route_input(struct sk_buff *skb);
82 struct dst_entry *ip6_route_input_lookup(struct net *net,
83 struct net_device *dev,
84 struct flowi6 *fl6,
85 const struct sk_buff *skb, int flags);
86
87 struct dst_entry *ip6_route_output_flags_noref(struct net *net,
88 const struct sock *sk,
89 struct flowi6 *fl6, int flags);
90
91 static inline struct dst_entry *ip6_route_output_flags(struct net *net,
92 const struct sock *sk,
93 struct flowi6 *fl6,
94 int flags) {
95 struct dst_entry *dst;
96 struct rt6_info *rt6;
97
98 rcu_read_lock();
99 dst = ip6_route_output_flags_noref(net, sk, fl6, flags);
100 rt6 = (struct rt6_info *)dst;
101 /* For dst cached in uncached_list, refcnt is already taken. */
102 if (list_empty(&rt6->rt6i_uncached) && !dst_hold_safe(dst)) {
> 103 dst = &net->ipv6.ip6_null_entry->dst;
104 dst_hold(dst);
105 }
106 rcu_read_unlock();
107
108 return dst;
109 }
110
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32662 bytes --]
^ permalink raw reply
* Re: [PATCH V33 24/30] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Matthew Garrett @ 2019-06-21 20:05 UTC (permalink / raw)
To: Andy Lutomirski
Cc: James Morris, linux-security, LKML, Linux API, David Howells,
Alexei Starovoitov, Network Development, Chun-Yi Lee,
Daniel Borkmann
In-Reply-To: <CALCETrVUwQP7roLnW6kFG80Cc5U6X_T6AW+BTAftLccYGp8+Ow@mail.gmail.com>
On Thu, Jun 20, 2019 at 10:22 PM Andy Lutomirski <luto@kernel.org> wrote:
> On Thu, Jun 20, 2019 at 6:21 PM Matthew Garrett
> <matthewgarrett@google.com> wrote:
> > --- a/security/lockdown/lockdown.c
> > +++ b/security/lockdown/lockdown.c
> > @@ -33,6 +33,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> > [LOCKDOWN_INTEGRITY_MAX] = "integrity",
> > [LOCKDOWN_KCORE] = "/proc/kcore access",
> > [LOCKDOWN_KPROBES] = "use of kprobes",
> > + [LOCKDOWN_BPF] = "use of bpf",
> > [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
>
> The text here says "use of bpf", but what this patch is *really* doing
> is locking down use of BPF to read kernel memory. If the details
> change, then every LSM needs to get updated, and we risk breaking user
> policies that are based on LSMs that offer excessively fine
> granularity.
The text is descriptive rather than normative, and no changes should
be made that alter the semantics of a reason - it makes more sense to
just add another reason.
> I'd be more comfortable if the LSM only got to see "confidentiality"
> or "integrity".
If LSM authors can be trusted to do the right thing here, then I see
no problem in providing additional data. I'm happy to defer to James
on that.
^ permalink raw reply
* Re: [PATCH v3 rdma-next 0/3] RDMA/qedr: Use the doorbell overflow recovery mechanism for RDMA
From: Jason Gunthorpe @ 2019-06-21 19:58 UTC (permalink / raw)
To: Michal Kalderon
Cc: Doug Ledford, Ariel Elior, linux-rdma@vger.kernel.org,
davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <MN2PR18MB3182498CA8C9C7EB3259F62FA1E70@MN2PR18MB3182.namprd18.prod.outlook.com>
On Fri, Jun 21, 2019 at 07:49:39PM +0000, Michal Kalderon wrote:
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Doug Ledford
> >
> > On Thu, 2019-06-13 at 11:38 +0300, Michal Kalderon wrote:
> > > This patch series used the doorbell overflow recovery mechanism
> > > introduced in commit 36907cd5cd72 ("qed: Add doorbell overflow
> > > recovery mechanism") for rdma ( RoCE and iWARP )
> > >
> > > rdma-core pull request #493
> > >
> > > Changes from V2:
> > > - Don't use long-lived kmap. Instead use user-trigger mmap for the
> > > doorbell recovery entries.
> > > - Modify dpi_addr to be denoted with __iomem and avoid redundant
> > > casts
> > >
> > > Changes from V1:
> > > - call kmap to map virtual address into kernel space
> > > - modify db_rec_delete to be void
> > > - remove some cpu_to_le16 that were added to previous patch which are
> > > correct but not related to the overflow recovery mechanism. Will be
> > > submitted as part of a different patch
> > >
> > >
> > > Michal Kalderon (3):
> > > qed*: Change dpi_addr to be denoted with __iomem
> > > RDMA/qedr: Add doorbell overflow recovery support
> > > RDMA/qedr: Add iWARP doorbell recovery support
> > >
> > > drivers/infiniband/hw/qedr/main.c | 2 +-
> > > drivers/infiniband/hw/qedr/qedr.h | 27 +-
> > > drivers/infiniband/hw/qedr/verbs.c | 387
> > > ++++++++++++++++++++++++-----
> > > drivers/net/ethernet/qlogic/qed/qed_rdma.c | 6 +-
> > > include/linux/qed/qed_rdma_if.h | 2 +-
> > > include/uapi/rdma/qedr-abi.h | 25 ++
> > > 6 files changed, 378 insertions(+), 71 deletions(-)
> > >
> >
> > Hi Michal,
> >
> > In patch 2 and 3 both, you still have quite a few casts to (u8 __iomem *).
> > Why not just define the struct elements as u8 __iomem * instead of void
> > __iomem * and avoid all the casts?
> >
> Hi Doug,
>
> Thanks for the review. The remaining casts are due to pointer arithmetic and not variable assignments
> as before. Removing the cast entirely will require quite a lot of changes in qed and in rdma-core
> which I would be happy to avoid at this time.
In linux pointer math on a void * acts the same as a u8 so you should
never need to cast a void * to a u8 just to do math?
Jason
^ permalink raw reply
* Re: [PATCH bpf-next v5 00/16] AF_XDP infrastructure improvements and mlx5e support
From: Saeed Mahameed @ 2019-06-21 19:52 UTC (permalink / raw)
To: Björn Töpel
Cc: Maxim Mikityanskiy, Alexei Starovoitov, Daniel Borkmann,
Björn Töpel, Magnus Karlsson, bpf@vger.kernel.org,
netdev@vger.kernel.org, David S. Miller, Saeed Mahameed,
Jonathan Lemon, Tariq Toukan, Martin KaFai Lau, Song Liu,
Yonghong Song, Jakub Kicinski, Maciej Fijalkowski
In-Reply-To: <CAJ+HfNia-vUv7Eumfs8aMYGGkxPbbUQ++F+BQ=9C1NtP0Jt3hA@mail.gmail.com>
On Thu, Jun 20, 2019 at 2:13 AM Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> On Tue, 18 Jun 2019 at 14:00, Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
> >
> > This series contains improvements to the AF_XDP kernel infrastructure
> > and AF_XDP support in mlx5e. The infrastructure improvements are
> > required for mlx5e, but also some of them benefit to all drivers, and
> > some can be useful for other drivers that want to implement AF_XDP.
> >
> > The performance testing was performed on a machine with the following
> > configuration:
> >
> > - 24 cores of Intel Xeon E5-2620 v3 @ 2.40 GHz
> > - Mellanox ConnectX-5 Ex with 100 Gbit/s link
> >
> > The results with retpoline disabled, single stream:
> >
> > txonly: 33.3 Mpps (21.5 Mpps with queue and app pinned to the same CPU)
> > rxdrop: 12.2 Mpps
> > l2fwd: 9.4 Mpps
> >
> > The results with retpoline enabled, single stream:
> >
> > txonly: 21.3 Mpps (14.1 Mpps with queue and app pinned to the same CPU)
> > rxdrop: 9.9 Mpps
> > l2fwd: 6.8 Mpps
> >
> > v2 changes:
> >
> > Added patches for mlx5e and addressed the comments for v1. Rebased for
> > bpf-next.
> >
> > v3 changes:
> >
> > Rebased for the newer bpf-next, resolved conflicts in libbpf. Addressed
> > Björn's comments for coding style. Fixed a bug in error handling flow in
> > mlx5e_open_xsk.
> >
> > v4 changes:
> >
> > UAPI is not changed, XSK RX queues are exposed to the kernel. The lower
> > half of the available amount of RX queues are regular queues, and the
> > upper half are XSK RX queues. The patch "xsk: Extend channels to support
> > combined XSK/non-XSK traffic" was dropped. The final patch was reworked
> > accordingly.
> >
> > Added "net/mlx5e: Attach/detach XDP program safely", as the changes
> > introduced in the XSK patch base on the stuff from this one.
> >
> > Added "libbpf: Support drivers with non-combined channels", which aligns
> > the condition in libbpf with the condition in the kernel.
> >
> > Rebased over the newer bpf-next.
> >
> > v5 changes:
> >
> > In v4, ethtool reports the number of channels as 'combined' and the
> > number of XSK RX queues as 'rx' for mlx5e. It was changed, so that 'rx'
> > is 0, and 'combined' reports the double amount of channels if there is
> > an active UMEM - to make libbpf happy.
> >
> > The patch for libbpf was dropped. Although it's still useful and fixes
> > things, it raises some disagreement, so I'm dropping it - it's no longer
> > useful for mlx5e anymore after the change above.
> >
>
> Just a heads-up: There are some checkpatch warnings (>80 chars/line)
Thanks Bjorn for your comment, in mlx5 we allow up to 95 chars per line,
otherwise it is going to be an ugly zigzags.
> for the mlnx5 driver parts, and the series didn't apply cleanly on
> bpf-next for me.
>
> I haven't been able to test the mlnx5 parts.
>
> Parts of the series are unrelated/orthogonal, and could be submitted
> as separate series, e.g. patches {1,7} and patches {3,4}. No blockers
> for me, though.
>
> Thanks for the hard work!
>
> For the series:
> Acked-by: Björn Töpel <bjorn.topel@intel.com>
^ permalink raw reply
* RE: [PATCH v3 rdma-next 0/3] RDMA/qedr: Use the doorbell overflow recovery mechanism for RDMA
From: Michal Kalderon @ 2019-06-21 19:49 UTC (permalink / raw)
To: Doug Ledford, Ariel Elior, jgg@ziepe.ca
Cc: linux-rdma@vger.kernel.org, davem@davemloft.net,
netdev@vger.kernel.org
In-Reply-To: <bda0321cb362bc93f5428b1df7daf69fed083656.camel@redhat.com>
> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Doug Ledford
>
> On Thu, 2019-06-13 at 11:38 +0300, Michal Kalderon wrote:
> > This patch series used the doorbell overflow recovery mechanism
> > introduced in commit 36907cd5cd72 ("qed: Add doorbell overflow
> > recovery mechanism") for rdma ( RoCE and iWARP )
> >
> > rdma-core pull request #493
> >
> > Changes from V2:
> > - Don't use long-lived kmap. Instead use user-trigger mmap for the
> > doorbell recovery entries.
> > - Modify dpi_addr to be denoted with __iomem and avoid redundant
> > casts
> >
> > Changes from V1:
> > - call kmap to map virtual address into kernel space
> > - modify db_rec_delete to be void
> > - remove some cpu_to_le16 that were added to previous patch which are
> > correct but not related to the overflow recovery mechanism. Will be
> > submitted as part of a different patch
> >
> >
> > Michal Kalderon (3):
> > qed*: Change dpi_addr to be denoted with __iomem
> > RDMA/qedr: Add doorbell overflow recovery support
> > RDMA/qedr: Add iWARP doorbell recovery support
> >
> > drivers/infiniband/hw/qedr/main.c | 2 +-
> > drivers/infiniband/hw/qedr/qedr.h | 27 +-
> > drivers/infiniband/hw/qedr/verbs.c | 387
> > ++++++++++++++++++++++++-----
> > drivers/net/ethernet/qlogic/qed/qed_rdma.c | 6 +-
> > include/linux/qed/qed_rdma_if.h | 2 +-
> > include/uapi/rdma/qedr-abi.h | 25 ++
> > 6 files changed, 378 insertions(+), 71 deletions(-)
> >
>
> Hi Michal,
>
> In patch 2 and 3 both, you still have quite a few casts to (u8 __iomem *).
> Why not just define the struct elements as u8 __iomem * instead of void
> __iomem * and avoid all the casts?
>
Hi Doug,
Thanks for the review. The remaining casts are due to pointer arithmetic and not variable assignments
as before. Removing the cast entirely will require quite a lot of changes in qed and in rdma-core
which I would be happy to avoid at this time.
Please reconsider,
Thanks again
Michal
> --
> Doug Ledford <dledford@redhat.com>
> GPG KeyID: B826A3330E572FDD
> Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
^ permalink raw reply
* Re: [PATCH net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Neil Horman @ 2019-06-21 19:18 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <CAF=yD-+MA398hTu7qCxfRhAMYrpeYp-+znc7bKNbupLYRRv5ug@mail.gmail.com>
On Fri, Jun 21, 2019 at 02:31:17PM -0400, Willem de Bruijn wrote:
> On Fri, Jun 21, 2019 at 12:42 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Thu, Jun 20, 2019 at 11:16:13AM -0400, Willem de Bruijn wrote:
> > > On Thu, Jun 20, 2019 at 10:24 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > On Thu, Jun 20, 2019 at 09:41:30AM -0400, Willem de Bruijn wrote:
> > > > > On Wed, Jun 19, 2019 at 4:26 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > >
> > > > > > When an application is run that:
> > > > > > a) Sets its scheduler to be SCHED_FIFO
> > > > > > and
> > > > > > b) Opens a memory mapped AF_PACKET socket, and sends frames with the
> > > > > > MSG_DONTWAIT flag cleared, its possible for the application to hang
> > > > > > forever in the kernel. This occurs because when waiting, the code in
> > > > > > tpacket_snd calls schedule, which under normal circumstances allows
> > > > > > other tasks to run, including ksoftirqd, which in some cases is
> > > > > > responsible for freeing the transmitted skb (which in AF_PACKET calls a
> > > > > > destructor that flips the status bit of the transmitted frame back to
> > > > > > available, allowing the transmitting task to complete).
> > > > > >
> > > > > > However, when the calling application is SCHED_FIFO, its priority is
> > > > > > such that the schedule call immediately places the task back on the cpu,
> > > > > > preventing ksoftirqd from freeing the skb, which in turn prevents the
> > > > > > transmitting task from detecting that the transmission is complete.
> > > > > >
> > > > > > We can fix this by converting the schedule call to a completion
> > > > > > mechanism. By using a completion queue, we force the calling task, when
> > > > > > it detects there are no more frames to send, to schedule itself off the
> > > > > > cpu until such time as the last transmitted skb is freed, allowing
> > > > > > forward progress to be made.
> > > > > >
> > > > > > Tested by myself and the reporter, with good results
> > > > > >
> > > > > > Appies to the net tree
> > > > > >
> > > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > > > Reported-by: Matteo Croce <mcroce@redhat.com>
> > > > > > CC: "David S. Miller" <davem@davemloft.net>
> > > > > > ---
> > > > >
> > > > > This is a complex change for a narrow configuration. Isn't a
> > > > > SCHED_FIFO process preempting ksoftirqd a potential problem for other
> > > > > networking workloads as well? And the right configuration to always
> > > > > increase ksoftirqd priority when increasing another process's
> > > > > priority? Also, even when ksoftirqd kicks in, isn't some progress
> > > > > still made on the local_bh_enable reached from schedule()?
> > > > >
> > > >
> > > > A few questions here to answer:
> > >
> > > Thanks for the detailed explanation.
> > >
> > > > Regarding other protocols having this problem, thats not the case, because non
> > > > packet sockets honor the SK_SNDTIMEO option here (i.e. they sleep for a period
> > > > of time specified by the SNDTIMEO option if MSG_DONTWAIT isn't set. We could
> > > > certainly do that, but the current implementation doesn't (opting instead to
> > > > wait indefinately until the respective packet(s) have transmitted or errored
> > > > out), and I wanted to maintain that behavior. If there is consensus that packet
> > > > sockets should honor SNDTIMEO, then I can certainly do that.
> > > >
> > > > As for progress made by calling local_bh_enable, My read of the code doesn't
> > > > have the scheduler calling local_bh_enable at all. Instead schedule uses
> > > > preempt_disable/preempt_enable_no_resched() to gain exlcusive access to the cpu,
> > > > which ignores pending softirqs on re-enablement.
> > >
> > > Ah, I'm mistaken there, then.
> > >
> > > > Perhaps that needs to change,
> > > > but I'm averse to making scheduler changes for this (the aforementioned concern
> > > > about complex changes for a narrow use case)
> > > >
> > > > Regarding raising the priority of ksoftirqd, that could be a solution, but the
> > > > priority would need to be raised to a high priority SCHED_FIFO parameter, and
> > > > that gets back to making complex changes for a narrow problem domain
> > > >
> > > > As for the comlexity of the of the solution, I think this is, given your
> > > > comments the least complex and intrusive change to solve the given problem.
> > >
> > > Could it be simpler to ensure do_softirq() gets run here? That would
> > > allow progress for this case.
> > >
> > > > We
> > > > need to find a way to force the calling task off the cpu while the asynchronous
> > > > operations in the transmit path complete, and we can do that this way, or by
> > > > honoring SK_SNDTIMEO. I'm fine with doing the latter, but I didn't want to
> > > > alter the current protocol behavior without consensus on that.
> > >
> > > In general SCHED_FIFO is dangerous with regard to stalling other
> > > progress, incl. ksoftirqd. But it does appear that this packet socket
> > > case is special inside networking in calling schedule() directly here.
> > >
> > > If converting that, should it convert to logic more akin to other
> > > sockets, like sock_wait_for_wmem? I haven't had a chance to read up on
> > > the pros and cons of completion here yet, sorry. Didn't want to delay
> > > responding until after I get a chance.
> > >
> > So, I started looking at implementing SOCK_SNDTIMEO for this patch, and
> > something occured to me....We still need a mechanism to block in tpacket_snd.
> > That is to say, other protocol use SK_SNDTIMEO to wait for socket memory to
> > become available, and that requirement doesn't exist for memory mapped sockets
> > in AF_PACKET (which tpacket_snd implements the kernel side for). We have memory
> > mapped frame buffers, which we marshall with an otherwise empty skb, and just
> > send that (i.e. no waiting on socket memory, we just product an error if we
> > don't have enough ram to allocate an sk_buff). Given that, we only ever need to
> > wait for a frame to complete transmission, or get freed in an error path further
> > down the stack. This probably explains why SK_SNDTIMEO doesn't exist for
> > AF_PACKET.
>
> SNDTIMEO behavior would still be useful: to wait for frame slot to
> become available, but only up to a timeout?
>
Ok, thats fair. To be clear, memory_mapped packets aren't waiting here for a
frame to become available for sending (thats done in userspace, where the
application checks a specific memory location for the TP_AVAILABLE status to be
set, so a new frame can be written). tpacket_snd is wating for the transmission
of a specific frame to complete the transmit action, which is a different thing.
Still, I suppose theres no reason we couldn't contrain that wait on a timeout
set by SK_SNDTIMEO
> To be clear, adding that is not a prerequisite for fixing this
> specific issue, of course. It would just be nice if the one happens to
> be fixed by adding the other.
>
> My main question is wrt the implementation details of struct
> completion. Without dynamic memory allocation,
> sock_wait_for_wmem/sk_stream_write_space obviously does not make
> sense. But should we still use sk_wq and more importantly does this
> need the same wait semantics (TASK_INTERRUPTIBLE) and does struct
> completion give those?
>
I suppose we could overload its use here, but I would be worried that such an
overload would be confusing. Nominally, sk_wq is used, as you note, to block
sockets whose allocated send space is full, until such time as enough frames
have been sent to make space for the next write. In this scenario, we already
know we have space to send a frame (by virtue of the fact that we are executing
in tpakcet_snd, which is only called after userspace has written a frame to the
memory mapped buffer already allocated for frame transmission). In this case we
are simply waiting for the last frame that we have sent to complete
transmission, at which point we can look to see if more frames are available to
send, or return from the system call. I'm happy to take an alternate consensus
into account, but for the sake of clarity I think I would rather use the
completion queue, as it makes clear the correlation between the waiter and the
event we are waiting on.
> >
> > Given that, is it worth implementing (as it will just further complicate this
> > patch, for no additional value), or can we move forward with it as it is?
> >
> > Neil
> >
>
^ permalink raw reply
* Re: [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function
From: Andrii Nakryiko @ 2019-06-21 19:13 UTC (permalink / raw)
To: Eelco Chaudron
Cc: Networking, Alexei Starovoitov, Daniel Borkmann, Martin Lau,
Song Liu, Yonghong Song
In-Reply-To: <49d3ddb42f531618584f60c740d9469e5406e114.1561130674.git.echaudro@redhat.com>
On Fri, Jun 21, 2019 at 8:26 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>
> When an AF_XDP application received X packets, it does not mean X
> frames can be stuffed into the producer ring. To make it easier for
> AF_XDP applications this API allows them to check how many frames can
> be added into the ring.
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
> tools/lib/bpf/xsk.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
> index 82ea71a0f3ec..86f3d485e957 100644
> --- a/tools/lib/bpf/xsk.h
> +++ b/tools/lib/bpf/xsk.h
> @@ -95,6 +95,12 @@ static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
> return r->cached_cons - r->cached_prod;
> }
>
> +static inline __u32 xsk_ring_prod__free(struct xsk_ring_prod *r)
This is a very bad name choice. __free is used for functions that free
memory and resources. One function below I see avail is used in the
name, why not xsk_ring_prog__avail?
> +{
> + r->cached_cons = *r->consumer + r->size;
> + return r->cached_cons - r->cached_prod;
> +}
> +
> static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32 nb)
> {
> __u32 entries = r->cached_prod - r->cached_cons;
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH 15/15] kbuild: compile test UAPI headers to ensure they are self-contained
From: Arnd Bergmann @ 2019-06-21 19:05 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linux Kbuild mailing list, Randy Dunlap, David Howells,
Sam Ravnborg, Jani Nikula, Song Liu, Alexei Starovoitov,
Networking, Yonghong Song, Linux Kernel Mailing List, linux-riscv,
Michal Marek, Martin KaFai Lau, Palmer Dabbelt, bpf,
Daniel Borkmann, Albert Ou
In-Reply-To: <20190604101409.2078-16-yamada.masahiro@socionext.com>
On Tue, Jun 4, 2019 at 12:16 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> --- a/Makefile
> +++ b/Makefile
> @@ -1363,7 +1363,7 @@ CLEAN_DIRS += $(MODVERDIR) include/ksym
> CLEAN_FILES += modules.builtin.modinfo
>
> # Directories & files removed with 'make mrproper'
> -MRPROPER_DIRS += include/config usr/include include/generated \
> +MRPROPER_DIRS += include/config include/generated \
> arch/$(SRCARCH)/include/generated .tmp_objdiff
> MRPROPER_FILES += .config .config.old .version \
> Module.symvers tags TAGS cscope* GPATH GTAGS GRTAGS GSYMS \
This change seems to have caused a minor regression:
$ make clean ; make clean
find: ‘*’: No such file or directory
Any idea?
Arnd
^ permalink raw reply
* Re: [PATCH net] ipv6: fix neighbour resolution with raw socket
From: kbuild test robot @ 2019-06-21 18:48 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: kbuild-all, davem, netdev, Nicolas Dichtel
In-Reply-To: <20190620123434.7219-1-nicolas.dichtel@6wind.com>
Hi Nicolas,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net/master]
url: https://github.com/0day-ci/linux/commits/Nicolas-Dichtel/ipv6-fix-neighbour-resolution-with-raw-socket/20190621-115455
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-rc1-7-g2b96cd8-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/net/vrf.c:363:17: sparse: sparse: incorrect type in assignment (different modifiers) @@ expected struct in6_addr *nexthop @@ got structstruct in6_addr *nexthop @@
>> drivers/net/vrf.c:363:17: sparse: expected struct in6_addr *nexthop
>> drivers/net/vrf.c:363:17: sparse: got struct in6_addr const *
include/net/route.h:356:48: sparse: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned int [usertype] key @@ got restrunsigned int [usertype] key @@
include/net/route.h:356:48: sparse: expected unsigned int [usertype] key
include/net/route.h:356:48: sparse: got restricted __be32 [usertype] daddr
include/net/route.h:356:48: sparse: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned int [usertype] key @@ got restrunsigned int [usertype] key @@
include/net/route.h:356:48: sparse: expected unsigned int [usertype] key
include/net/route.h:356:48: sparse: got restricted __be32 [usertype] daddr
--
>> net/bluetooth/6lowpan.c:188:25: sparse: sparse: incorrect type in assignment (different modifiers) @@ expected struct in6_addr *[assigned] nexthop @@ got t in6_addr *[assigned] nexthop @@
>> net/bluetooth/6lowpan.c:188:25: sparse: expected struct in6_addr *[assigned] nexthop
>> net/bluetooth/6lowpan.c:188:25: sparse: got struct in6_addr const *
vim +363 drivers/net/vrf.c
dcdd43c4 David Ahern 2017-03-20 345
35402e31 David Ahern 2015-10-12 346 #if IS_ENABLED(CONFIG_IPV6)
35402e31 David Ahern 2015-10-12 347 /* modelled after ip6_finish_output2 */
35402e31 David Ahern 2015-10-12 348 static int vrf_finish_output6(struct net *net, struct sock *sk,
35402e31 David Ahern 2015-10-12 349 struct sk_buff *skb)
35402e31 David Ahern 2015-10-12 350 {
35402e31 David Ahern 2015-10-12 351 struct dst_entry *dst = skb_dst(skb);
35402e31 David Ahern 2015-10-12 352 struct net_device *dev = dst->dev;
35402e31 David Ahern 2015-10-12 353 struct neighbour *neigh;
35402e31 David Ahern 2015-10-12 354 struct in6_addr *nexthop;
35402e31 David Ahern 2015-10-12 355 int ret;
35402e31 David Ahern 2015-10-12 356
eb63ecc1 David Ahern 2016-12-14 357 nf_reset(skb);
eb63ecc1 David Ahern 2016-12-14 358
35402e31 David Ahern 2015-10-12 359 skb->protocol = htons(ETH_P_IPV6);
35402e31 David Ahern 2015-10-12 360 skb->dev = dev;
35402e31 David Ahern 2015-10-12 361
35402e31 David Ahern 2015-10-12 362 rcu_read_lock_bh();
35402e31 David Ahern 2015-10-12 @363 nexthop = rt6_nexthop((struct rt6_info *)dst, &ipv6_hdr(skb)->daddr);
35402e31 David Ahern 2015-10-12 364 neigh = __ipv6_neigh_lookup_noref(dst->dev, nexthop);
35402e31 David Ahern 2015-10-12 365 if (unlikely(!neigh))
35402e31 David Ahern 2015-10-12 366 neigh = __neigh_create(&nd_tbl, nexthop, dst->dev, false);
35402e31 David Ahern 2015-10-12 367 if (!IS_ERR(neigh)) {
4ff06203 Julian Anastasov 2017-02-06 368 sock_confirm_neigh(skb, neigh);
0353f282 David Ahern 2019-04-05 369 ret = neigh_output(neigh, skb, false);
35402e31 David Ahern 2015-10-12 370 rcu_read_unlock_bh();
35402e31 David Ahern 2015-10-12 371 return ret;
35402e31 David Ahern 2015-10-12 372 }
35402e31 David Ahern 2015-10-12 373 rcu_read_unlock_bh();
35402e31 David Ahern 2015-10-12 374
35402e31 David Ahern 2015-10-12 375 IP6_INC_STATS(dev_net(dst->dev),
35402e31 David Ahern 2015-10-12 376 ip6_dst_idev(dst), IPSTATS_MIB_OUTNOROUTES);
35402e31 David Ahern 2015-10-12 377 kfree_skb(skb);
35402e31 David Ahern 2015-10-12 378 return -EINVAL;
35402e31 David Ahern 2015-10-12 379 }
35402e31 David Ahern 2015-10-12 380
:::::: The code at line 363 was first introduced by commit
:::::: 35402e31366349a32b505afdfe856aeeb8d939a0 net: Add IPv6 support to VRF device
:::::: TO: David Ahern <dsa@cumulusnetworks.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ 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