* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Tejun Heo @ 2012-08-24 20:33 UTC (permalink / raw)
To: Sasha Levin
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, lw-BthXqXjhjHXQFUHtdCDX3A,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <5037E00B.6090606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hello, Sasha.
On Fri, Aug 24, 2012 at 10:11:55PM +0200, Sasha Levin wrote:
> > If this implementation is about the common trivial case, why not just
> > have the usual DECLARE/DEFINE_HASHTABLE() combination?
>
> When we add the dynamic non-resizable support, how would DEFINE_HASHTABLE() look?
Hmmm? DECLARE/DEFINE are usually for static ones.
> > I don't know. If we stick to the static (or even !resize dymaic)
> > straight-forward hash - and we need something like that - I don't see
> > what the full encapsulation buys us other than a lot of trivial
> > wrappers.
>
> Which macros do you consider as trivial within the current API?
>
> Basically this entire thing could be reduced to DEFINE/DECLARE_HASHTABLE and
> get_bucket(), but it would make the life of anyone who wants a slightly
> different hashtable a hell.
Wouldn't the following be enough to get most of the benefits?
* DECLARE/DEFINE
* hash_head()
* hash_for_each_head()
* hash_add*()
* hash_for_each_possible*()
> I think that right now the only real trivial wrapper is hash_hashed(), and I
> think it's a price worth paying to have a single hashtable API instead of
> fragmenting it when more implementations come along.
I'm not objecting strongly against full encapsulation but having this
many thin wrappers makes me scratch my head.
Thanks.
--
tejun
^ permalink raw reply
* Re: pull request: sfc-next 2012-08-24
From: David Miller @ 2012-08-24 20:36 UTC (permalink / raw)
To: bhutchings; +Cc: linux-net-drivers, netdev
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 24 Aug 2012 20:46:14 +0100
> The following changes since commit 8f4cccbbd92f2ad0ddbbc498ef7cee2a1c3defe9:
>
> net: Set device operstate at registration time (2012-08-24 12:46:13 -0400)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next.git for-davem
>
> (commit 8f8b3d518999fd1c342310910aa1e49112c86d05)
>
> 1. Change the TX path to stop queues earlier and avoid returning
> NETDEV_TX_BUSY.
> 2. Remove some inefficiencies in soft-TSO.
> 3. Fix various bugs involving device state transitions and/or reset
> scheduling by error handlers.
> 4. Take advantage of my previous change to operstate initialisation.
> 5. Miscellaneous cleanup.
Looks good, pulled, thanks Ben.
^ permalink raw reply
* Re: [linuxppc-release] [PATCH] net/fsl_pq_mdio: add support for the Fman 1G MDIO controller
From: David Miller @ 2012-08-24 20:37 UTC (permalink / raw)
To: timur; +Cc: afleming, netdev
In-Reply-To: <5037E445.9040102@freescale.com>
From: Timur Tabi <timur@freescale.com>
Date: Fri, 24 Aug 2012 15:29:57 -0500
> David Miller wrote:
>>> >
>>> > This patch isn't quite right. I'll post a new version soon.
>
>> I already applied it to net-next, didn't you see?
>
> You applied this patch:
>
> http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commit;h=9f35a7342cff0be72e3c038ea972e07662ca1ce8
Aha, my bad. :)
^ permalink raw reply
* Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges
From: Stephen Hemminger @ 2012-08-24 20:44 UTC (permalink / raw)
To: vyasevic; +Cc: Nicolas de Pesloüan, netdev
In-Reply-To: <5036EC52.9090406@redhat.com>
On Thu, 23 Aug 2012 22:52:02 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:
> On 08/23/2012 05:03 PM, Nicolas de Pesloüan wrote:
> > Le 23/08/2012 21:29, Vlad Yasevich a écrit :
> >> This series of patches provides an ability to add VLAN IDs to the bridge
> >> ports. This is similar to what can be found in most switches. The
> >> bridge
> >> port may have any number of VLANs added to it including vlan 0 for
> >> untagged
> >> traffic. When vlans are added to the port, only traffic tagged with
> >> particular
> >> vlan will forwarded over this port. Additionally, vlan ids are added
> >> to FDB
> >> entries and become part of the lookup. This way we correctly identify
> >> the FDB
> >> entry.
> >>
> >> There are still pieces missing. I don't yet support adding a static
> >> fdb entry
> >> with a particular vlan. There is no netlink support for carrying a
> >> vlan id.
> >>
> >> I'd like to hear thoughts of whether this is usufull and something we
> >> should
> >> persue.
> >>
> >
> > Do you think this might allow for per VLAN spanning tree (having ports
> > in forwarding state or blocking state depending on the VLAN) in the future?
> >
> > Nicolas.
>
> sure, why not.
The vlan map table would be helpful, but the Spanning Tree implementation
doesn't have a clue about what it means.
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-24 20:53 UTC (permalink / raw)
To: Tejun Heo
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, lw-BthXqXjhjHXQFUHtdCDX3A,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120824203332.GF21325-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On 08/24/2012 10:33 PM, Tejun Heo wrote:
> Hello, Sasha.
>
> On Fri, Aug 24, 2012 at 10:11:55PM +0200, Sasha Levin wrote:
>>> If this implementation is about the common trivial case, why not just
>>> have the usual DECLARE/DEFINE_HASHTABLE() combination?
>>
>> When we add the dynamic non-resizable support, how would DEFINE_HASHTABLE() look?
>
> Hmmm? DECLARE/DEFINE are usually for static ones.
Yup, but we could be using the same API for dynamic non-resizable and static if
we go with the DECLARE/hash_init. We could switch between them (and other
implementations) without having to change the code.
>>> I don't know. If we stick to the static (or even !resize dymaic)
>>> straight-forward hash - and we need something like that - I don't see
>>> what the full encapsulation buys us other than a lot of trivial
>>> wrappers.
>>
>> Which macros do you consider as trivial within the current API?
>>
>> Basically this entire thing could be reduced to DEFINE/DECLARE_HASHTABLE and
>> get_bucket(), but it would make the life of anyone who wants a slightly
>> different hashtable a hell.
>
> Wouldn't the following be enough to get most of the benefits?
>
> * DECLARE/DEFINE
> * hash_head()
> * hash_for_each_head()
> * hash_add*()
> * hash_for_each_possible*()
* hash_for_each*() ?
Why do we need hash_head/hash_for_each_head()? I haven't stumbled on a place yet
that needed direct access to the bucket itself.
Consider the following list:
- DECLARE
- hash_init
- hash_add
- hash_del
- hash_hashed
- hash_for_each_[rcu, safe]
- hash_for_each_possible[rcu, safe]
This basically means 11 macros/functions that would let us have full
encapsulation and will make it very easy for future implementations to work with
this API instead of making up a new one. It's also not significantly (+~2-3)
more than the ones you listed.
>> I think that right now the only real trivial wrapper is hash_hashed(), and I
>> think it's a price worth paying to have a single hashtable API instead of
>> fragmenting it when more implementations come along.
>
> I'm not objecting strongly against full encapsulation but having this
> many thin wrappers makes me scratch my head.
>
> Thanks.
>
^ permalink raw reply
* Re: [PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 5).
From: Nicholas A. Bellinger @ 2012-08-24 20:56 UTC (permalink / raw)
To: Naresh Kumar Inna
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <5037BC94.1050201@chelsio.com>
On Fri, 2012-08-24 at 23:10 +0530, Naresh Kumar Inna wrote:
> On 8/24/2012 1:18 AM, Nicholas A. Bellinger wrote:
> > On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
> >> This patch contains code to implement the interrupt handling and the fast
> >> path I/O functionality. The interrupt handling includes allocation of
> >> MSIX vectors, registering and implemeting the interrupt service routines.
> >> The fast path I/O functionality includes posting the I/O request to firmware
> >> via Work Requests, tracking/completing them, and handling task management
> >> requests. SCSI midlayer host template implementation is also covered by
> >> this patch.
> >>
> >> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
> >> ---
> >
> > Hi Naresh,
> >
> > My review comments are inline below..
>
> Hi Nicholas,
>
> Thanks for taking the time to review the driver. Please find my replies
> inline.
>
> Regards,
> Naresh.
>
> >
> >> drivers/scsi/csiostor/csio_isr.c | 631 ++++++++++
> >> drivers/scsi/csiostor/csio_scsi.c | 2498 +++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 3129 insertions(+), 0 deletions(-)
> >> create mode 100644 drivers/scsi/csiostor/csio_isr.c
> >> create mode 100644 drivers/scsi/csiostor/csio_scsi.c
> >>
<SNIP>
> >
> >> diff --git a/drivers/scsi/csiostor/csio_scsi.c b/drivers/scsi/csiostor/csio_scsi.c
> >> new file mode 100644
> >> index 0000000..0f87b00
> >> --- /dev/null
> >> +++ b/drivers/scsi/csiostor/csio_scsi.c
> >> +
> >> +#define CSIO_SCSI_CMD_WR_SZ(_imm) \
> >> + (sizeof(struct fw_scsi_cmd_wr) + /* WR size */ \
> >> + ALIGN((_imm), 16)) /* Immed data */
> >> +
> >> +#define CSIO_SCSI_CMD_WR_SZ_16(_imm) \
> >> + (ALIGN(CSIO_SCSI_CMD_WR_SZ((_imm)), 16))
> >> +
> >> +/*
> >> + * csio_scsi_cmd - Create a SCSI CMD WR.
> >> + * @req: IO req structure.
> >> + *
> >> + * Gets a WR slot in the ingress queue and initializes it with SCSI CMD WR.
> >> + *
> >> + */
> >> +static inline void
> >> +csio_scsi_cmd(struct csio_ioreq *req)
> >> +{
> >> + struct csio_wr_pair wrp;
> >> + struct csio_hw *hw = req->lnode->hwp;
> >> + struct csio_scsim *scsim = csio_hw_to_scsim(hw);
> >> + uint32_t size = CSIO_SCSI_CMD_WR_SZ_16(scsim->proto_cmd_len);
> >> +
> >> + req->drv_status = csio_wr_get(hw, req->eq_idx, size, &wrp);
> >> + if (unlikely(req->drv_status != CSIO_SUCCESS))
> >> + return;
> >> +
> >> + if (wrp.size1 >= size) {
> >> + /* Initialize WR in one shot */
> >> + csio_scsi_init_cmd_wr(req, wrp.addr1, size);
> >> + } else {
> >> + uint8_t tmpwr[512];
> >
> > Mmmm, putting this large of a buffer on the local stack is probably not
> > a good idea.
> >
> > This should become an allocation.. If it's a hot path then you'll
> > probably want to set this up before-hand.
> >
>
> The else switch above is entered only when we near the end of the DMA
> ring. This is not a frequent occurrence. If it is a strict no-no to have
> so many on-stack bytes, I have to think of a way re-work it to use
> pre-allocated memory. Please let me know.
>
Considering it's used a number of times, it would be better to just
figure out a sensible manner to pre-allocate this, especially if it's
only a single occurrence to an individual ring held under a lock.
> >> + /*
> >> + * Make a temporary copy of the WR and write back
> >> + * the copy into the WR pair.
> >> + */
> >> + csio_scsi_init_cmd_wr(req, (void *)tmpwr, size);
> >> + memcpy(wrp.addr1, tmpwr, wrp.size1);
> >> + memcpy(wrp.addr2, tmpwr + wrp.size1, size - wrp.size1);
> >> + }
> >> +}
> >> +
> >> +/*
> >> + * The following is fast path code. Therefore it is inlined with multi-line
> >> + * macros using name substitution, thus avoiding if-else switches for
> >> + * operation (read/write), as well as serving the purpose of code re-use.
> >> + */
> >> +/*
> >> + * csio_scsi_init_ulptx_dsgl - Fill in a ULP_TX_SC_DSGL
> >> + * @hw: HW module
> >> + * @req: IO request
> >> + * @sgl: ULP TX SGL pointer.
> >> + *
> >> + */
> >> +#define csio_scsi_init_ultptx_dsgl(hw, req, sgl) \
> >> +do { \
> >> + struct ulptx_sge_pair *_sge_pair = NULL; \
> >> + struct scatterlist *_sgel; \
> >> + uint32_t _i = 0; \
> >> + uint32_t _xfer_len; \
> >> + struct list_head *_tmp; \
> >> + struct csio_dma_buf *_dma_buf; \
> >> + struct scsi_cmnd *scmnd = csio_scsi_cmnd((req)); \
> >> + \
> >> + (sgl)->cmd_nsge = htonl(ULPTX_CMD(ULP_TX_SC_DSGL) | ULPTX_MORE | \
> >> + ULPTX_NSGE((req)->nsge)); \
> >> + /* Now add the data SGLs */ \
> >> + if (likely(!(req)->dcopy)) { \
> >> + scsi_for_each_sg(scmnd, _sgel, (req)->nsge, _i) { \
> >> + if (_i == 0) { \
> >> + (sgl)->addr0 = cpu_to_be64( \
> >> + sg_dma_address(_sgel)); \
> >> + (sgl)->len0 = cpu_to_be32( \
> >> + sg_dma_len(_sgel)); \
> >> + _sge_pair = \
> >> + (struct ulptx_sge_pair *)((sgl) + 1); \
> >> + continue; \
> >> + } \
> >> + if ((_i - 1) & 0x1) { \
> >> + _sge_pair->addr[1] = cpu_to_be64( \
> >> + sg_dma_address(_sgel)); \
> >> + _sge_pair->len[1] = cpu_to_be32( \
> >> + sg_dma_len(_sgel)); \
> >> + _sge_pair++; \
> >> + } else { \
> >> + _sge_pair->addr[0] = cpu_to_be64( \
> >> + sg_dma_address(_sgel)); \
> >> + _sge_pair->len[0] = cpu_to_be32( \
> >> + sg_dma_len(_sgel)); \
> >> + } \
> >> + } \
> >> + } else { \
> >> + /* Program sg elements with driver's DDP buffer */ \
> >> + _xfer_len = scsi_bufflen(scmnd); \
> >> + list_for_each(_tmp, &(req)->gen_list) { \
> >> + _dma_buf = (struct csio_dma_buf *)_tmp; \
> >> + if (_i == 0) { \
> >> + (sgl)->addr0 = cpu_to_be64(_dma_buf->paddr); \
> >> + (sgl)->len0 = cpu_to_be32( \
> >> + min(_xfer_len, _dma_buf->len)); \
> >> + _sge_pair = \
> >> + (struct ulptx_sge_pair *)((sgl) + 1); \
> >> + } \
> >> + else if ((_i - 1) & 0x1) { \
> >> + _sge_pair->addr[1] = cpu_to_be64( \
> >> + _dma_buf->paddr); \
> >> + _sge_pair->len[1] = cpu_to_be32( \
> >> + min(_xfer_len, _dma_buf->len)); \
> >> + _sge_pair++; \
> >> + } else { \
> >> + _sge_pair->addr[0] = cpu_to_be64( \
> >> + _dma_buf->paddr); \
> >> + _sge_pair->len[0] = cpu_to_be32( \
> >> + min(_xfer_len, _dma_buf->len)); \
> >> + } \
> >> + _xfer_len -= min(_xfer_len, _dma_buf->len); \
> >> + _i++; \
> >> + } \
> >> + } \
> >> +} while (0)
> >> +
> >
> > I don't see any reason why this can't just be a static function..? Why
> > is the macro usage necessary here..?
>
> OK, I will make this static-inline.
>
> >
> >> +/*
> >> + * csio_scsi_init_data_wr - Initialize the READ/WRITE SCSI WR.
> >> + * @req: IO req structure.
> >> + * @oper: read/write
> >> + * @wrp: DMA location to place the payload.
> >> + * @size: Size of WR (including FW WR + immed data + rsp SG entry + data SGL
> >> + * @wrop: _READ_/_WRITE_
> >> + *
> >> + * Wrapper for populating fw_scsi_read_wr/fw_scsi_write_wr.
> >> + */
> >> +#define csio_scsi_init_data_wr(req, oper, wrp, size, wrop) \
> >> +do { \
> >> + struct csio_hw *_hw = (req)->lnode->hwp; \
> >> + struct csio_rnode *_rn = (req)->rnode; \
> >> + struct fw_scsi_##oper##_wr *__wr = (struct fw_scsi_##oper##_wr *)(wrp);\
> >> + struct ulptx_sgl *_sgl; \
> >> + struct csio_dma_buf *_dma_buf; \
> >> + uint8_t _imm = csio_hw_to_scsim(_hw)->proto_cmd_len; \
> >> + struct scsi_cmnd *scmnd = csio_scsi_cmnd((req)); \
> >> + \
> >> + __wr->op_immdlen = cpu_to_be32(FW_WR_OP(FW_SCSI##wrop##WR) | \
> >> + FW_SCSI##wrop##WR_IMMDLEN(_imm)); \
> >> + __wr->flowid_len16 = cpu_to_be32(FW_WR_FLOWID(_rn->flowid) | \
> >> + FW_WR_LEN16( \
> >> + CSIO_ROUNDUP((size), 16))); \
> >> + __wr->cookie = (uintptr_t) (req); \
> >> + __wr->iqid = (uint16_t)cpu_to_be16(csio_q_physiqid(_hw, \
> >> + (req)->iq_idx));\
> >> + __wr->tmo_val = (uint8_t)((req)->tmo); \
> >> + __wr->use_xfer_cnt = 1; \
> >> + __wr->xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
> >> + __wr->ini_xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
> >> + /* Get RSP DMA buffer */ \
> >> + _dma_buf = &(req)->dma_buf; \
> >> + \
> >> + /* Prepare RSP SGL */ \
> >> + __wr->rsp_dmalen = cpu_to_be32(_dma_buf->len); \
> >> + __wr->rsp_dmaaddr = cpu_to_be64(_dma_buf->paddr); \
> >> + \
> >> + __wr->r4 = 0; \
> >> + \
> >> + __wr->u.fcoe.ctl_pri = 0; \
> >> + __wr->u.fcoe.cp_en_class = 0; \
> >> + __wr->u.fcoe.r3_lo[0] = 0; \
> >> + __wr->u.fcoe.r3_lo[1] = 0; \
> >> + csio_scsi_fcp_cmnd((req), (void *)((uintptr_t)(wrp) + \
> >> + sizeof(struct fw_scsi_##oper##_wr))); \
> >> + \
> >> + /* Move WR pointer past command and immediate data */ \
> >> + _sgl = (struct ulptx_sgl *) ((uintptr_t)(wrp) + \
> >> + sizeof(struct fw_scsi_##oper##_wr) + \
> >> + ALIGN(_imm, 16)); \
> >> + \
> >> + /* Fill in the DSGL */ \
> >> + csio_scsi_init_ultptx_dsgl(_hw, (req), _sgl); \
> >> + \
> >> +} while (0)
> >> +
> >
> > This one has four uses of CPP keys. Just turn those into macros, and
> > leave the rest of the code in a static function.
> >
>
> So what you are suggesting is to have all the lines of the macro
> csio_scsi_init_data_wr() added into a static function, but for the ones
> with the 4 keys. csio_scsi_init_data_wr() will then invoke this new
> function. Is that correct?
>
Not sure how the above should actually look without actually doing it,
but IMHO the usage of macro just obfuscates what is going on..
If it's only used a few times, just inline the code into seperate static
functions. If it's used more than a few times, then use a single static
funciton with macro accessors for the assignment of the various '__wr'
structure members.
The larger problem with all of these macros is that you can't tell what
is a macro and what is a function.
If you need to use a CPP macro, please make sure to capitalize the name
of the macro in order to tell the difference between the two.
> >> +/* Calculate WR size needed for fw_scsi_read_wr/fw_scsi_write_wr */
> >> +#define csio_scsi_data_wrsz(req, oper, sz, imm) \
> >> +do { \
> >> + (sz) = sizeof(struct fw_scsi_##oper##_wr) + /* WR size */ \
> >> + ALIGN((imm), 16) + /* Immed data */ \
> >> + sizeof(struct ulptx_sgl); /* ulptx_sgl */ \
> >> + \
> >> + if (unlikely((req)->nsge > 1)) \
> >> + (sz) += (sizeof(struct ulptx_sge_pair) * \
> >> + (ALIGN(((req)->nsge - 1), 2) / 2)); \
> >> + /* Data SGE */ \
> >> +} while (0)
> >> +
> >> +/*
> >> + * csio_scsi_data - Create a SCSI WRITE/READ WR.
> >> + * @req: IO req structure.
> >> + * @oper: read/write
> >> + * @wrop: _READ_/_WRITE_ (string subsitutions to use with the FW bit field
> >> + * macros).
> >> + *
> >> + * Gets a WR slot in the ingress queue and initializes it with
> >> + * SCSI CMD READ/WRITE WR.
> >> + *
> >> + */
> >> +#define csio_scsi_data(req, oper, wrop) \
> >> +do { \
> >> + struct csio_wr_pair _wrp; \
> >> + uint32_t _size; \
> >> + struct csio_hw *_hw = (req)->lnode->hwp; \
> >> + struct csio_scsim *_scsim = csio_hw_to_scsim(_hw); \
> >> + \
> >> + csio_scsi_data_wrsz((req), oper, _size, _scsim->proto_cmd_len); \
> >> + _size = ALIGN(_size, 16); \
> >> + \
> >> + (req)->drv_status = csio_wr_get(_hw, (req)->eq_idx, _size, &_wrp); \
> >> + if (likely((req)->drv_status == CSIO_SUCCESS)) { \
> >> + if (likely(_wrp.size1 >= _size)) { \
> >> + /* Initialize WR in one shot */ \
> >> + csio_scsi_init_data_wr((req), oper, _wrp.addr1, \
> >> + _size, wrop); \
> >> + } else { \
> >> + uint8_t tmpwr[512]; \
> >> + /* \
> >> + * Make a temporary copy of the WR and write back \
> >> + * the copy into the WR pair. \
> >> + */ \
> >> + csio_scsi_init_data_wr((req), oper, (void *)tmpwr, \
> >> + _size, wrop); \
> >> + memcpy(_wrp.addr1, tmpwr, _wrp.size1); \
> >> + memcpy(_wrp.addr2, tmpwr + _wrp.size1, \
> >> + _size - _wrp.size1); \
> >> + } \
> >> + } \
> >> +} while (0)
> >> +
> >
> > Ditto on this one, along with the tmpwr[512] stack usage..
> >
> >> +static inline void
> >> +csio_scsi_abrt_cls(struct csio_ioreq *req, bool abort)
> >> +{
> >> + struct csio_wr_pair wrp;
> >> + struct csio_hw *hw = req->lnode->hwp;
> >> + uint32_t size = ALIGN(sizeof(struct fw_scsi_abrt_cls_wr), 16);
> >> +
> >> + req->drv_status = csio_wr_get(hw, req->eq_idx, size, &wrp);
> >> + if (req->drv_status != CSIO_SUCCESS)
> >> + return;
> >> +
> >> + if (wrp.size1 >= size) {
> >> + /* Initialize WR in one shot */
> >> + csio_scsi_init_abrt_cls_wr(req, wrp.addr1, size, abort);
> >> + } else {
> >> + uint8_t tmpwr[512];
> >
> > Ditto here on local scope stack usage..
> >
> >> + /*
> >> + * Make a temporary copy of the WR and write back
> >> + * the copy into the WR pair.
> >> + */
> >> + csio_scsi_init_abrt_cls_wr(req, (void *)tmpwr, size, abort);
> >> + memcpy(wrp.addr1, tmpwr, wrp.size1);
> >> + memcpy(wrp.addr2, tmpwr + wrp.size1, size - wrp.size1);
> >> + }
> >> +}
> >> +
> >> +/*****************************************************************************/
> >> +/* START: SCSI SM */
> >> +/*****************************************************************************/
> >> +static void
> >> +csio_scsis_uninit(struct csio_ioreq *req, enum csio_scsi_ev evt)
> >> +{
> >> + struct csio_hw *hw = req->lnode->hwp;
> >> + struct csio_scsim *scsim = csio_hw_to_scsim(hw);
> >> +
> >> + switch (evt) {
> >> +
> >> + case CSIO_SCSIE_START_IO:
> >
> > Extra space between start of first switch case
>
> OK, I will remove it. Is there any tool that catches such deviations?
> checkpath.pl didnt report it.
>
Sorry, can't help you there.. ;)
> >
> >> +
> >> +/**
> >> + * csio_queuecommand_lck - Entry point to kickstart an I/O request.
> >> + * @cmnd: The I/O request from ML.
> >> + * @done: The ML callback routine.
> >> + *
> >> + * This routine does the following:
> >> + * - Checks for HW and Rnode module readiness.
> >> + * - Gets a free ioreq structure (which is already initialized
> >> + * to uninit during its allocation).
> >> + * - Maps SG elements.
> >> + * - Initializes ioreq members.
> >> + * - Kicks off the SCSI state machine for this IO.
> >> + * - Returns busy status on error.
> >> + */
> >> +static int
> >> +csio_queuecommand_lck(struct scsi_cmnd *cmnd, void (*done)(struct scsi_cmnd *))
> >> +{
> >> + struct csio_lnode *ln = shost_priv(cmnd->device->host);
> >> + struct csio_hw *hw = csio_lnode_to_hw(ln);
> >> + struct csio_scsim *scsim = csio_hw_to_scsim(hw);
> >> + struct csio_rnode *rn = (struct csio_rnode *)(cmnd->device->hostdata);
> >> + struct csio_ioreq *ioreq = NULL;
> >> + unsigned long flags;
> >> + int nsge = 0;
> >> + int rv = SCSI_MLQUEUE_HOST_BUSY, nr;
> >> + csio_retval_t retval;
> >> + int cpu;
> >> + struct csio_scsi_qset *sqset;
> >> + struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
> >> +
> >> + if (!blk_rq_cpu_valid(cmnd->request))
> >> + cpu = smp_processor_id();
> >> + else
> >> + cpu = cmnd->request->cpu;
> >> +
> >> + sqset = &hw->sqset[ln->portid][cpu];
> >> +
> >> + nr = fc_remote_port_chkready(rport);
> >> + if (nr) {
> >> + cmnd->result = nr;
> >> + csio_inc_stats(scsim, n_rn_nr_error);
> >> + goto err_done;
> >> + }
> >> +
> >> + if (unlikely(!csio_is_hw_ready(hw))) {
> >> + cmnd->result = (DID_REQUEUE << 16);
> >> + csio_inc_stats(scsim, n_hw_nr_error);
> >> + goto err_done;
> >> + }
> >> +
> >> + /* Get req->nsge, if there are SG elements to be mapped */
> >> + nsge = scsi_dma_map(cmnd);
> >> + if (unlikely(nsge < 0)) {
> >> + csio_inc_stats(scsim, n_dmamap_error);
> >> + goto err;
> >> + }
> >> +
> >> + /* Do we support so many mappings? */
> >> + if (unlikely(nsge > scsim->max_sge)) {
> >> + csio_warn(hw,
> >> + "More SGEs than can be supported."
> >> + " SGEs: %d, Max SGEs: %d\n", nsge, scsim->max_sge);
> >> + csio_inc_stats(scsim, n_unsupp_sge_error);
> >> + goto err_dma_unmap;
> >> + }
> >> +
> >> + /* Get a free ioreq structure - SM is already set to uninit */
> >> + ioreq = csio_get_scsi_ioreq_lock(hw, scsim);
> >> + if (!ioreq) {
> >> + csio_err(hw, "Out of I/O request elements. Active #:%d\n",
> >> + scsim->stats.n_active);
> >> + csio_inc_stats(scsim, n_no_req_error);
> >> + goto err_dma_unmap;
> >> + }
> >> +
> >> + ioreq->nsge = nsge;
> >> + ioreq->lnode = ln;
> >> + ioreq->rnode = rn;
> >> + ioreq->iq_idx = sqset->iq_idx;
> >> + ioreq->eq_idx = sqset->eq_idx;
> >> + ioreq->wr_status = 0;
> >> + ioreq->drv_status = CSIO_SUCCESS;
> >> + csio_scsi_cmnd(ioreq) = (void *)cmnd;
> >> + ioreq->tmo = 0;
> >> +
> >> + switch (cmnd->sc_data_direction) {
> >> + case DMA_BIDIRECTIONAL:
> >> + ioreq->datadir = CSIO_IOREQF_DMA_BIDI;
> >> + csio_inc_stats(ln, n_control_requests);
> >> + break;
> >> + case DMA_TO_DEVICE:
> >> + ioreq->datadir = CSIO_IOREQF_DMA_WRITE;
> >> + csio_inc_stats(ln, n_output_requests);
> >> + ln->stats.n_output_bytes += scsi_bufflen(cmnd);
> >> + break;
> >> + case DMA_FROM_DEVICE:
> >> + ioreq->datadir = CSIO_IOREQF_DMA_READ;
> >> + csio_inc_stats(ln, n_input_requests);
> >> + ln->stats.n_input_bytes += scsi_bufflen(cmnd);
> >> + break;
> >> + case DMA_NONE:
> >> + ioreq->datadir = CSIO_IOREQF_DMA_NONE;
> >> + csio_inc_stats(ln, n_control_requests);
> >> + break;
> >> + default:
> >> + CSIO_DB_ASSERT(0);
> >> + break;
> >> + }
> >> +
> >> + /* Set cbfn */
> >> + ioreq->io_cbfn = csio_scsi_cbfn;
> >> +
> >> + /* Needed during abort */
> >> + cmnd->host_scribble = (unsigned char *)ioreq;
> >> + cmnd->scsi_done = done;
> >> + cmnd->SCp.Message = 0;
> >> +
> >> + /* Kick off SCSI IO SM on the ioreq */
> >> + spin_lock_irqsave(&hw->lock, flags);
> >> + retval = csio_scsi_start_io(ioreq);
> >> + spin_unlock_irqrestore(&hw->lock, flags);
> >> +
> >> + if (retval != CSIO_SUCCESS) {
> >> + csio_err(hw, "ioreq: %p couldnt be started, status:%d\n",
> >> + ioreq, retval);
> >> + csio_inc_stats(scsim, n_busy_error);
> >> + goto err_put_req;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +err_put_req:
> >> + csio_put_scsi_ioreq_lock(hw, scsim, ioreq);
> >> +err_dma_unmap:
> >> + if (nsge > 0)
> >> + scsi_dma_unmap(cmnd);
> >> +err:
> >> + return rv;
> >> +
> >> +err_done:
> >> + done(cmnd);
> >> + return 0;
> >> +}
> >> +
> >> +static DEF_SCSI_QCMD(csio_queuecommand);
> >> +
> >
> > This means that your running with the host_lock held.. I'm not sure if
> > that is really what you want to do as it really end's up killing
> > multi-lun small packet performance..
> >
> > How about dropping DEF_SCSI_QCMD usage here, and figure out what
> > actually needs to be protected by the SCSI host_lock within
> > csio_queuecommand_lck()..?
>
> It is on my TODO list for the next version of the driver, after the
> initial submission. Per the current design, we shouldnt need the
> host_lock to be held, but I would like to test this change thoroughly
> before I submit it.
>
Sure, it's really quite easy to convert and these days the majority of
high performance LLDs do run in host-lock-less mode.
IIRC the libfc based FCoE initiator driver is doing this too..
^ permalink raw reply
* Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges
From: Nicolas de Pesloüan @ 2012-08-24 21:09 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: vyasevic, netdev
In-Reply-To: <20120824134410.75827e0d@nehalam.linuxnetplumber.net>
Le 24/08/2012 22:44, Stephen Hemminger a écrit :
<snip>
> The vlan map table would be helpful, but the Spanning Tree implementation
> doesn't have a clue about what it means.
Yes, but a possibly future userland implementation of MSTP (or PVST) would benefit from the ability
to manage per VLAN port states.
So at least we should have this in mind while integrating VLAN into the bridge code.
Nicolas.
^ permalink raw reply
* Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).
From: Nicholas A. Bellinger @ 2012-08-24 21:17 UTC (permalink / raw)
To: Naresh Kumar Inna
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <5037C997.4030803@chelsio.com>
On Sat, 2012-08-25 at 00:06 +0530, Naresh Kumar Inna wrote:
> On 8/24/2012 1:28 AM, Nicholas A. Bellinger wrote:
> > On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
> >> This patch contains the first set of the header files for csiostor driver.
> >>
> >> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
> >> ---
> >> drivers/scsi/csiostor/csio_defs.h | 143 ++++++
> >> drivers/scsi/csiostor/csio_fcoe_proto.h | 843 +++++++++++++++++++++++++++++++
> >> drivers/scsi/csiostor/csio_hw.h | 668 ++++++++++++++++++++++++
> >> drivers/scsi/csiostor/csio_init.h | 158 ++++++
> >> 4 files changed, 1812 insertions(+), 0 deletions(-)
> >> create mode 100644 drivers/scsi/csiostor/csio_defs.h
> >> create mode 100644 drivers/scsi/csiostor/csio_fcoe_proto.h
> >> create mode 100644 drivers/scsi/csiostor/csio_hw.h
> >> create mode 100644 drivers/scsi/csiostor/csio_init.h
> >>
> >
> > Hi Naresh,
> >
> > Just commenting on csio_defs.h bits here... As Robert mentioned, you'll
> > need to convert the driver to use (or add to) upstream protocol
> > definitions and drop the csio_fcoe_proto.h bits..
> >
>
> Hi Nicholas,
>
> I would like take up the discussion of the protocol header file in that
> email thread. Please find the rest of my replies inline.
>
> Thanks for reviewing,
> Naresh.
>
> >> diff --git a/drivers/scsi/csiostor/csio_defs.h b/drivers/scsi/csiostor/csio_defs.h
> >> new file mode 100644
> >> index 0000000..4f1c713
> >> --- /dev/null
> >> +++ b/drivers/scsi/csiostor/csio_defs.h
<SNIP>
> >> +#ifndef __CSIO_DEFS_H__
> >> +#define __CSIO_DEFS_H__
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/timer.h>
> >> +#include <linux/list.h>
> >> +#include <linux/bug.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/jiffies.h>
> >> +
> >> +/* Function returns */
> >> +enum csio_retval {
> >> + CSIO_SUCCESS = 0,
> >> + CSIO_INVAL = 1,
> >> + CSIO_BUSY = 2,
> >> + CSIO_NOSUPP = 3,
> >> + CSIO_TIMEOUT = 4,
> >> + CSIO_NOMEM = 5,
> >> + CSIO_NOPERM = 6,
> >> + CSIO_RETRY = 7,
> >> + CSIO_EPROTO = 8,
> >> + CSIO_EIO = 9,
> >> + CSIO_CANCELLED = 10,
> >> +};
> >> +
> >
> > Please don't assign macros for errno's, and give them positive values.
> >
>
> Although some of these return values appear to be mapped to errno
> values, there are others that do not have an errno equivalent (example
> CSIO_CANCELLED). We may have future needs to have more driver/protocol
> specific return values as well. What do you suggest?
>
Convert all functions aside from CSIO_CANCELLED to use normal negative
return values from include/asm-generic/error[-base].h
For the CSIO_CANCELLED case, propagate this status up to the specific
caller using another method..
> >> +#define csio_retval_t enum csio_retval
> >
> > Please get rid of this csio_retval_t nonsense.
>
> I can get rid of the typedef and use enum csio_retval instead.
>
Using a LLD defined retval where %90 of the items are from errno.h is
code duplication. Please get rid of this.
> >
> >> +
> >> +enum {
> >> + CSIO_FALSE = 0,
> >> + CSIO_TRUE = 1,
> >> +};
> >> +
> >
> > Same here, please use normal Boolean macros
> >
> >> +#define CSIO_ROUNDUP(__v, __r) (((__v) + (__r) - 1) / (__r))
> >> +#define CSIO_INVALID_IDX 0xFFFFFFFF
> >> +#define csio_inc_stats(elem, val) ((elem)->stats.val++)
> >> +#define csio_dec_stats(elem, val) ((elem)->stats.val--)
> >
> > No reason for either of this stats inc+dec macros. Please drop them.
>
> I will get rid of them.
>
> >
> >> +#define csio_valid_wwn(__n) ((*__n >> 4) == 0x5 ? CSIO_TRUE : \
> >> + CSIO_FALSE)
> >> +#define CSIO_WORD_TO_BYTE 4
> >> +
> >> +static inline int
> >> +csio_list_deleted(struct list_head *list)
> >> +{
> >> + return ((list->next == list) && (list->prev == list));
> >> +}
> >> +
> >> +#define csio_list_next(elem) (((struct list_head *)(elem))->next)
> >> +#define csio_list_prev(elem) (((struct list_head *)(elem))->prev)
> >> +
> >> +#define csio_deq_from_head(head, elem) \
> >> +do { \
> >> + if (!list_empty(head)) { \
> >> + *((struct list_head **)(elem)) = csio_list_next((head)); \
> >> + csio_list_next((head)) = \
> >> + csio_list_next(csio_list_next((head))); \
> >> + csio_list_prev(csio_list_next((head))) = (head); \
> >> + INIT_LIST_HEAD(*((struct list_head **)(elem))); \
> >> + } else \
> >> + *((struct list_head **)(elem)) = (struct list_head *)NULL;\
> >> +} while (0)
> >> +
> >
> > This code is confusing as hell.. Why can't you just use normal list.h
> > macros for this..?
>
> I have not found an equivalent function in list.h that does the above
> and the following macro. Could you please point me to it? I have seen a
> couple of other drivers define their own macros to achieve what this
> macro does, hence I assumed there isnt a list.h macro that does this.
>
AFAICT all that csio_deq_from_head code is supposed to do is pull an
item off a list, right..? Why not just:
while (!list_empty(list)) {
elem = list_first_entry(list, struct elem_type,
elm_list);
list_del_init(&elem->elm_list);
<do work>
<free *elem memory>
}
> >> +#define csio_deq_from_tail(head, elem) \
> >> +do { \
> >> + if (!list_empty(head)) { \
> >> + *((struct list_head **)(elem)) = csio_list_prev((head)); \
> >> + csio_list_prev((head)) = \
> >> + csio_list_prev(csio_list_prev((head))); \
> >> + csio_list_next(csio_list_prev((head))) = (head); \
> >> + INIT_LIST_HEAD(*((struct list_head **)(elem))); \
> >> + } else \
> >> + *((struct list_head **)(elem)) = (struct list_head *)NULL;\
> >> +} while (0)
> >> +
> >
> > Same here.. Please don't use macros like this.
> >
AFIACT csio_deq_from_tail is unused..?
Please remove it..
^ permalink raw reply
* Re: [net-next 04/13] e1000e: cleanup checkpatch PREFER_PR_LEVEL warning
From: Jeff Kirsher @ 2012-08-24 21:19 UTC (permalink / raw)
To: Joe Perches, Allan, Bruce W; +Cc: David Miller, netdev, gospo, sassmann
In-Reply-To: <1345831348.32359.8.camel@joe2Laptop>
[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]
On Fri, 2012-08-24 at 11:02 -0700, Joe Perches wrote:
> On Thu, 2012-08-23 at 07:01 -0700, Joe Perches wrote:
> > On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:
> > > From: Bruce Allan <bruce.w.allan@intel.com>
> > >
> > > checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...
> > []
> > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > []
> > > @@ -4330,9 +4330,8 @@ static void e1000_print_link_info(struct e1000_adapter *adapter)
> > > u32 ctrl = er32(CTRL);
> > >
> > > /* Link status message must follow this format for user tools */
> > > - printk(KERN_INFO "e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
> > > - adapter->netdev->name,
> > > - adapter->link_speed,
> > > + pr_info("e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
> > > + adapter->netdev->name, adapter->link_speed,
> > > adapter->link_duplex == FULL_DUPLEX ? "Full" : "Half",
> > > (ctrl & E1000_CTRL_TFCE) && (ctrl & E1000_CTRL_RFCE) ? "Rx/Tx" :
> > > (ctrl & E1000_CTRL_RFCE) ? "Rx" :
> >
> > I think these conversions are not a good idea.
> >
> > When you have a specific message format that must be
> > followed, use printk.
> >
> > pr_<level> may at some point in the near future use
> > #define pr_fmt(fmt) KBUiLD_MODNAME ": " fmt
> > as a global default equivalent.
>
> Hey Jeff.
>
> The comment above this change (and the other) reads
>
> /* Link status message must follow this format for user tools */
>
> This file already uses #define pr_fmt(fmt) KBUILD_MODNAME...
> With this patch, the output form changes to use 2 prefixes.
>
> Is that really desired? Probably not.
>
> If the comments are old and don't apply any more, they
> should be removed.
>
Bruce really should answer this since this is his patch and there was a
reason why he made the change. My guess was the current output was
providing incorrect or mis-leading information.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Tejun Heo @ 2012-08-24 21:23 UTC (permalink / raw)
To: Sasha Levin
Cc: torvalds, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, mathieu.desnoyers, axboe, agk, dm-devel, neilb,
ccaulfie, teigland, Trond.Myklebust, bfields, fweisbec, jesse,
venkat.x.venkatsubra, ejt, snitzer, edumazet, linux-nfs, dev,
rds-devel, lw
In-Reply-To: <5037E9D9.9000605@gmail.com>
Hello,
On Fri, Aug 24, 2012 at 10:53:45PM +0200, Sasha Levin wrote:
> Yup, but we could be using the same API for dynamic non-resizable and static if
> we go with the DECLARE/hash_init. We could switch between them (and other
> implementations) without having to change the code.
I think it's better to stick with the usual conventions.
> > * DECLARE/DEFINE
> > * hash_head()
> > * hash_for_each_head()
> > * hash_add*()
> > * hash_for_each_possible*()
> * hash_for_each*() ?
>
> Why do we need hash_head/hash_for_each_head()? I haven't stumbled on a place yet
> that needed direct access to the bucket itself.
Because whole hash table walking is much less common and we can avoid
another full set of iterators.
> This basically means 11 macros/functions that would let us have full
> encapsulation and will make it very easy for future implementations to work with
> this API instead of making up a new one. It's also not significantly (+~2-3)
> more than the ones you listed.
I'm not sure whether full encapsulation is a good idea for trivial
hashtable. For higher level stuff, sure but at this level I think
benefits coming from known obvious implementation can be larger.
e.g. suppose the caller knows certain entries to be way colder than
others and wants to put them at the end of the chain.
So, I think implmenting the minimal set of helpers which reflect the
underlying trivial implementation explicitly could actually be better
even when discounting the reduced number of wrappers.
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH 0/8] csiostor: Chelsio FCoE offload driver submission
From: Paul Gortmaker @ 2012-08-24 21:45 UTC (permalink / raw)
To: Naresh Kumar Inna; +Cc: JBottomley, linux-scsi, dm, netdev, chethan
In-Reply-To: <1345760873-12101-1-git-send-email-naresh@chelsio.com>
On Thu, Aug 23, 2012 at 6:27 PM, Naresh Kumar Inna <naresh@chelsio.com> wrote:
> This is the initial submission of the Chelsio FCoE offload driver (csiostor)
> to the upstream kernel. This driver currently supports FCoE offload
> functionality over Chelsio T4-based 10Gb Converged Network Adapters.
>
> The following patches contain the driver sources for csiostor driver and
> updates to firmware/hardware header files shared between csiostor and
> cxgb4 (Chelsio T4-based NIC driver). The csiostor driver is dependent on these
> header updates. These patches have been generated against scsi 'misc' branch.
>
> csiostor is a low level SCSI driver that interfaces with PCI, SCSI midlayer and
> FC transport subsystems. This driver claims the FCoE PCIe function on the
> Chelsio Converged Network Adapter. It relies on firmware events for slow path
> operations like discovery, thereby offloading session management. The driver
> programs firmware via Work Request interfaces for fast path I/O offload
> features.
>
> Here is the brief description of patches:
> [PATCH 1/8]: Hardware interface, Makefile and Kconfig changes.
> [PATCH 2/8]: Driver initialization and Work Request services.
> [PATCH 3/8]: FC transport interfaces and mailbox services.
> [PATCH 4/8]: Local and remote port state tracking functionality.
> [PATCH 5/8]: Interrupt handling and fast path I/O functionality.
> [PATCH 6/8]: Header files part 1.
> [PATCH 7/8]: Header files part 2.
Based on the above two, I'm guessing nothing will build and work
on any of steps one through six? Yet you expose the Kconfig
and Makefile linkage into the tree in patch #1? So your patches
as presented are non bisectable.
You need to rethink your breakup in the presentation. Factoring
things just by files alone is not the right approach. You need to
ask yourself whether each commit is a stand-alone entity that does
something independently on its own -- since they generally should.
> [PATCH 8/8]: Updates to header files shared between cxgb4 and csiostor.
>
> Naresh Kumar Inna (8):
> csiostor: Chelsio FCoE offload driver submission (sources part 1).
> csiostor: Chelsio FCoE offload driver submission (sources part 2).
> csiostor: Chelsio FCoE offload driver submission (sources part 3).
> csiostor: Chelsio FCoE offload driver submission (sources part 4).
> csiostor: Chelsio FCoE offload driver submission (sources part 5).
> csiostor: Chelsio FCoE offload driver submission (headers part 1).
> csiostor: Chelsio FCoE offload driver submission (headers part 2).
> cxgb4: Chelsio FCoE offload driver submission (cxgb4 common header
> updates).
Something is wrong here. You offer up semi-informational shortlogs
in your 1st list, but this shows the shortlogs you've used in the actual
commits are largely content-free.
The above are just what I'd happened to notice in reading the 0/8
since I was curious what it was for. I've not looked at the individual
driver code itself. It might be worthwhile to go over some of the
suggestions in Documentation/Submit* files though.
Paul.
^ permalink raw reply
* Re: [PATCH 0/8] csiostor: Chelsio FCoE offload driver submission
From: James Bottomley @ 2012-08-24 21:58 UTC (permalink / raw)
To: Paul Gortmaker; +Cc: Naresh Kumar Inna, linux-scsi, dm, netdev, chethan
In-Reply-To: <CAP=VYLqtcULyoxPv0z89SEFvvFJiBUpG0UhWXzfm8KVMoGmYpg@mail.gmail.com>
On Fri, 2012-08-24 at 17:45 -0400, Paul Gortmaker wrote:
> On Thu, Aug 23, 2012 at 6:27 PM, Naresh Kumar Inna <naresh@chelsio.com> wrote:
> > This is the initial submission of the Chelsio FCoE offload driver (csiostor)
> > to the upstream kernel. This driver currently supports FCoE offload
> > functionality over Chelsio T4-based 10Gb Converged Network Adapters.
> >
> > The following patches contain the driver sources for csiostor driver and
> > updates to firmware/hardware header files shared between csiostor and
> > cxgb4 (Chelsio T4-based NIC driver). The csiostor driver is dependent on these
> > header updates. These patches have been generated against scsi 'misc' branch.
> >
> > csiostor is a low level SCSI driver that interfaces with PCI, SCSI midlayer and
> > FC transport subsystems. This driver claims the FCoE PCIe function on the
> > Chelsio Converged Network Adapter. It relies on firmware events for slow path
> > operations like discovery, thereby offloading session management. The driver
> > programs firmware via Work Request interfaces for fast path I/O offload
> > features.
> >
> > Here is the brief description of patches:
> > [PATCH 1/8]: Hardware interface, Makefile and Kconfig changes.
> > [PATCH 2/8]: Driver initialization and Work Request services.
> > [PATCH 3/8]: FC transport interfaces and mailbox services.
> > [PATCH 4/8]: Local and remote port state tracking functionality.
> > [PATCH 5/8]: Interrupt handling and fast path I/O functionality.
> > [PATCH 6/8]: Header files part 1.
> > [PATCH 7/8]: Header files part 2.
>
> Based on the above two, I'm guessing nothing will build and work
> on any of steps one through six? Yet you expose the Kconfig
> and Makefile linkage into the tree in patch #1? So your patches
> as presented are non bisectable.
>
> You need to rethink your breakup in the presentation. Factoring
> things just by files alone is not the right approach. You need to
> ask yourself whether each commit is a stand-alone entity that does
> something independently on its own -- since they generally should.
Actually, this is fine for a new driver ... everything just gets put in
as a single commit. There's really no point adding a driver in pieces
if it's not functional until the last patch, so in this case only, the
breakup is to help the review.
James
^ permalink raw reply
* Re: [net-next 04/13] e1000e: cleanup checkpatch PREFER_PR_LEVEL warning
From: Joe Perches @ 2012-08-24 22:43 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: Allan, Bruce W, David Miller, netdev, gospo, sassmann
In-Reply-To: <1345843164.3613.9.camel@jtkirshe-mobl>
On Fri, 2012-08-24 at 14:19 -0700, Jeff Kirsher wrote:
> On Fri, 2012-08-24 at 11:02 -0700, Joe Perches wrote:
> > On Thu, 2012-08-23 at 07:01 -0700, Joe Perches wrote:
> > > On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:
> > > > From: Bruce Allan <bruce.w.allan@intel.com>
> > > >
> > > > checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...
> > > []
> > > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > > []
> > > > @@ -4330,9 +4330,8 @@ static void e1000_print_link_info(struct e1000_adapter *adapter)
> > > > u32 ctrl = er32(CTRL);
> > > >
> > > > /* Link status message must follow this format for user tools */
> > > > - printk(KERN_INFO "e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
> > > > - adapter->netdev->name,
> > > > - adapter->link_speed,
> > > > + pr_info("e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
> > > > + adapter->netdev->name, adapter->link_speed,
> > > > adapter->link_duplex == FULL_DUPLEX ? "Full" : "Half",
> > > > (ctrl & E1000_CTRL_TFCE) && (ctrl & E1000_CTRL_RFCE) ? "Rx/Tx" :
> > > > (ctrl & E1000_CTRL_RFCE) ? "Rx" :
[]
> > The comment above this change (and the other) reads
> >
> > /* Link status message must follow this format for user tools */
> >
> > This file already uses #define pr_fmt(fmt) KBUILD_MODNAME...
> > With this patch, the output form changes to use 2 prefixes.
> >
> > Is that really desired? Probably not.
> >
> > If the comments are old and don't apply any more, they
> > should be removed.
> >
> Bruce really should answer this since this is his patch and there was a
> reason why he made the change. My guess was the current output was
> providing incorrect or mis-leading information.
My guess is he was just shutting up checkpatch and
didn't notice the newly doubled prefix.
happy weekend...
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-24 22:59 UTC (permalink / raw)
To: Tejun Heo
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, lw-BthXqXjhjHXQFUHtdCDX3A,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120824212348.GK21325-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> Why do we need hash_head/hash_for_each_head()? I haven't stumbled on a place yet
>> that needed direct access to the bucket itself.
>
> Because whole hash table walking is much less common and we can avoid
> another full set of iterators.
I don't agree. Out of 32 places which now use a hashtable iterator of some kind,
12 of them (38%) walk the entire table.
The thing is that usually data structures are indexable by more than one key, so
usually hashtables are fully walked in cold paths to look for different keys.
Take kernel/workqueue.c for example: There are 4 places which do a key lookup
(find_worker_executing_work()) and 3 places which fully walk the entire table
(for_each_busy_worker()).
>> This basically means 11 macros/functions that would let us have full
>> encapsulation and will make it very easy for future implementations to work with
>> this API instead of making up a new one. It's also not significantly (+~2-3)
>> more than the ones you listed.
>
> I'm not sure whether full encapsulation is a good idea for trivial
> hashtable. For higher level stuff, sure but at this level I think
> benefits coming from known obvious implementation can be larger.
> e.g. suppose the caller knows certain entries to be way colder than
> others and wants to put them at the end of the chain.
Thats the thing, the amount of things of things you can do with a given bucket
is very limited. You can't add entries to any point besides the head (without
walking the entire list).
Basically you can do only two things with a bucket:
- Add something to it at a very specific place.
- Walk it
So I don't understand whats the point in exposing the internal structure of the
hashtable if there's nothing significant that can be gained from it by the user.
>
> So, I think implmenting the minimal set of helpers which reflect the
> underlying trivial implementation explicitly could actually be better
> even when discounting the reduced number of wrappers.
>
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Tejun Heo @ 2012-08-24 23:07 UTC (permalink / raw)
To: Sasha Levin
Cc: torvalds, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, mathieu.desnoyers, axboe, agk, dm-devel, neilb,
ccaulfie, teigland, Trond.Myklebust, bfields, fweisbec, jesse,
venkat.x.venkatsubra, ejt, snitzer, edumazet, linux-nfs, dev,
rds-devel, lw
In-Reply-To: <5038074D.300@gmail.com>
Hello,
On Sat, Aug 25, 2012 at 12:59:25AM +0200, Sasha Levin wrote:
> Thats the thing, the amount of things of things you can do with a given bucket
> is very limited. You can't add entries to any point besides the head (without
> walking the entire list).
Kinda my point. We already have all the hlist*() interface to deal
with such cases. Having something which is evidently the trivial
hlist hashtable and advertises as such in the interface can be
helpful. I think we need that more than we need anything fancy.
Heh, this is a debate about which one is less insignificant. I can
see your point. I'd really like to hear what others think on this.
Guys, do we want something which is evidently trivial hlist hashtable
which can use hlist_*() API directly or do we want something better
encapsulated?
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v2 00/10] cgroup: Assign subsystem IDs during compile time
From: Tejun Heo @ 2012-08-24 23:15 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <1345816904-21745-1-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
Hello, Daniel.
On Fri, Aug 24, 2012 at 04:01:34PM +0200, Daniel Wagner wrote:
> CGROUP_BUILTIN_SUBSYS_COUNT is also gone in this version. This time I
> trade space for speed. Some extra cycles are spend to identify the
> modules in the for loops, e.g.
There's no point in optimizing either space or speed here. It's not a
hot path by any stretch of imagination. Please focus on making it
simple.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 01/10] cgroup: net_cls: Use empty task_cls_classid() when !CONFIG_NET_CLS(_MODULE)
From: Tejun Heo @ 2012-08-24 23:22 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Gao feng, Jamal Hadi Salim,
John Fastabend, Li Zefan, Neil Horman
In-Reply-To: <1345816904-21745-2-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
On Fri, Aug 24, 2012 at 04:01:35PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
How about,
When CONFIG_NET_CLS_CGROUP is N, task_cls_classid() doesn't
have any user but the module version still gets defined. Move
it inside IS_MODULE(CONFIG_NET_CLS_CGROUP).
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 03/10] cgroup: net_cls: Protect access to task_cls_classid() when built as module
From: Tejun Heo @ 2012-08-24 23:26 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Gao feng, Jamal Hadi Salim,
John Fastabend, Li Zefan, Neil Horman
In-Reply-To: <1345816904-21745-4-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote:
> @@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void)
> synchronize_rcu();
> #endif
>
> +#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
> + static_key_slow_dec(&cgroup_cls_enabled);
> + rcu_barrier();
Why is this rcu_barrier() necessary? In general, please explain what
synchronization is going on when using sync constructs which aren't
obvious - e.g. memory barriers, rcu barriers.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 04/10] cgroup: net_prio: Protect access to task_netprioidx() when built as module
From: Tejun Heo @ 2012-08-24 23:26 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Gao feng, Jamal Hadi Salim,
John Fastabend, Li Zefan, Neil Horman
In-Reply-To: <1345816904-21745-5-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
On Fri, Aug 24, 2012 at 04:01:38PM +0200, Daniel Wagner wrote:
> +#if IS_MODULE(CONFIG_NETPRIO_CGROUP)
> + static_key_slow_dec(&cgroup_netprio_enabled);
> + rcu_barrier();
Ditto as patch 3.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 05/10] cgroup: Remove CGROUP_BUILTIN_SUBSYS_COUNT
From: Tejun Heo @ 2012-08-24 23:28 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, Li Zefan, David S. Miller
In-Reply-To: <1345816904-21745-6-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
On Fri, Aug 24, 2012 at 04:01:39PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>
> CGROUP_BUILTIN_SUBSYS_COUNT is used as start or stop point
> when looping over the subsys array. Since the subsys array is
> 64 entries long this is a good thing to do. Though we'd like
> to reduce the array size considerable but we need to get rid
> of CGROUP_BUILTIN_SUBSYS_COUNT to ease up the review process.
Wouldn't it be better to explicitly state that a following patch would
reduce the SUBSYS_COUNT and stop putting builtin and module ones into
different sections?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 06/10] cgroup: Assign subsystem IDs during compile time
From: Tejun Heo @ 2012-08-24 23:38 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Neil Horman
In-Reply-To: <1345816904-21745-7-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
Hello,
On Fri, Aug 24, 2012 at 04:01:40PM +0200, Daniel Wagner wrote:
> We are able to safe some space when we assign the subsystem
^ ^
save if we assign both builtin and
module susbsystem IDs at compile time?
> IDs at compile time. Instead of allocating per cgroup
> cgroup->subsys[CGROUP_SUBSYS_COUNT] where CGROUP_SUBSYS_COUNT is
> always 64, we allocate at max 12 (at this point there are 12
> subsystem).
Please note (in big fat ugly way) that this disallows support for
modular controller which isn't known at kernel compile time.
> task_cls_classid() and task_netprioidx() (when built as
> module) are protected by a jump label and therefore we can
> simply replace the subsystem index lookup with the enum.
Can we put these in a separate patch? ie. The first patch makes all
subsys IDs constant and then patches to simplify users.
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 3787872..ada517f 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -43,18 +43,27 @@ extern void cgroup_unload_subsys(struct cgroup_subsys *ss);
>
> extern const struct file_operations proc_cgroup_operations;
>
> -/* Define the enumeration of all builtin cgroup subsystems */
> -#define SUBSYS(_x) _x ## _subsys_id,
> +/*
> + * Define the enumeration of all builtin cgroup subsystems.
> + * For the builtin subsystems the subsys_id needs to be indentical
> + * with the index in css->subsys. Therefore, all the builtin
> + * subsys are listed first and then the modules ids.
> + */
> enum cgroup_subsys_id {
> +#define SUBSYS(_x) _x ## _subsys_id,
> +
> +#define IS_SUBSYS_ENABLED(option) IS_BUILTIN(option)
> #include <linux/cgroup_subsys.h>
> -};
> +#undef IS_SUBSYS_ENABLED
> +
> +#define IS_SUBSYS_ENABLED(option) IS_MODULE(option)
> +#include <linux/cgroup_subsys.h>
> +#undef IS_SUBSYS_ENABLED
> +
Why do we need to segregate in-kernel and modular ones at all? What's
wrong with just defining them in one go?
> diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
> index 24443d2..43fae13 100644
> --- a/include/net/cls_cgroup.h
> +++ b/include/net/cls_cgroup.h
> @@ -49,22 +49,16 @@ static inline u32 task_cls_classid(struct task_struct *p)
> extern struct static_key cgroup_cls_enabled;
> #define clscg_enabled static_key_false(&cgroup_cls_enabled)
>
> -extern int net_cls_subsys_id;
> -
> static inline u32 task_cls_classid(struct task_struct *p)
> {
> - int id;
> - u32 classid = 0;
> + u32 classid;
>
> if (!clscg_enabled || in_interrupt())
> return 0;
>
> rcu_read_lock();
> - id = rcu_dereference_index_check(net_cls_subsys_id,
> - rcu_read_lock_held());
> - if (id >= 0)
> - classid = container_of(task_subsys_state(p, id),
> - struct cgroup_cls_state, css)->classid;
> + classid = container_of(task_subsys_state(p, net_cls_subsys_id),
> + struct cgroup_cls_state, css)->classid;
> rcu_read_unlock();
>
> return classid;
Hmm... patch sequence looks odd to me. If you first make all IDs
constant, you can first remove module specific ones and then later add
jump labels as separate patches. Wouldn't that be simpler?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 07/10] cgroup: net_cls: Simplify ifdef logic
From: Tejun Heo @ 2012-08-24 23:39 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Gao feng, Jamal Hadi Salim,
John Fastabend, Li Zefan, Neil Horman
In-Reply-To: <1345816904-21745-8-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
On Fri, Aug 24, 2012 at 04:01:41PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
I suppose this is equivalent conversion? Maybe it would be nice to
note that in patch description?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 08/10] cgroup: net_cls: Merge builtin and module version of task_cls_classid()
From: Tejun Heo @ 2012-08-24 23:41 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Gao feng, Jamal Hadi Salim,
John Fastabend, Li Zefan, Neil Horman
In-Reply-To: <1345816904-21745-9-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
On Fri, Aug 24, 2012 at 04:01:42PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
How about,
Now that all cgroup subsys IDs are constant whether a subsys
is built-in or modular, there's no reason to have separate
versions of task_cls_classid(). Unify them.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 09/10] cgroup: net_prio: Simplify ifdef logic
From: Tejun Heo @ 2012-08-24 23:42 UTC (permalink / raw)
To: Daniel Wagner
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Gao feng, Jamal Hadi Salim,
John Fastabend, Li Zefan, Neil Horman
In-Reply-To: <1345816904-21745-10-git-send-email-wagi-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
On Fri, Aug 24, 2012 at 04:01:43PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
Okay, for a few trivial patches, $SUBJ as description is fine but this
series is pushing it too far. Is this equivalent conversion? Why
does sock_update_netprioidx() earn an addition ifdef around it? :(
--
tejun
^ permalink raw reply
* Re: [PATCH] iproute2: Add FDB print and update cmds for self and master
From: Stephen Hemminger @ 2012-08-25 0:11 UTC (permalink / raw)
To: John Fastabend; +Cc: netdev, vyasevic
In-Reply-To: <20120823203719.6544.22863.stgit@jf-dev1-dcblab>
On Thu, 23 Aug 2012 13:37:19 -0700
John Fastabend <john.r.fastabend@intel.com> wrote:
> Add command to update and print FDB entries with NTF_SELF and
> NTF_MASTER set.
>
> Example usages illustrating use of 'self' to program embedded
> forwarding table and 'master' to configure the forwarding table
> of the bridge. Also shows 'master self' used to update both in
> the same command.
>
> #./br/br fdb add 00:1b:21:55:23:60 dev eth3 self
> #./br/br fdb add 00:1b:21:55:23:60 dev eth3 master
> #./br/br fdb add 00:1b:21:55:23:61 dev eth3 self master
> #./br/br fdb add 00:1b:21:55:23:62 dev eth3
> #./br/br fdb show
> eth3 00:1b:21:55:23:60 local self
> eth3 00:1b:21:55:23:61 local self
> eth3 33:33:00:00:00:01 local self
> eth3 01:00:5e:00:00:01 local self
> eth3 33:33:ff:55:23:59 local self
> eth3 01:00:5e:00:00:fb local self
> eth33 33:33:00:00:00:01 local self
> eth34 33:33:00:00:00:01 local self
> eth3 00:1b:21:55:23:59 local master
> eth3 00:1b:21:55:23:60 static master
> eth3 00:1b:21:55:23:62 static master
> eth3 00:1b:21:55:23:61 static master
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Applied
^ 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